[PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 258 bytes --]

The last two /dev/disk subdirectories :-)

Note a minor difference: On Linux, empty /dev/disk subdirectories 
apparently never appear. A subdirectory is not listed in /dev/disk if it 
would be empty. Not worth the effort to emulate.

-- 
Regards,
Christian


[-- Attachment #2: 0001-Cygwin-Add-dev-disk-by-label-and-dev-disk-by-uuid-sy.patch --]
[-- Type: text/plain, Size: 8249 bytes --]

From 27f4a10431aacbdc9725571f4ad34732081fb63a Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Fri, 17 Nov 2023 15:22:52 +0100
Subject: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

The new directories '/dev/disk/by-label' and '/dev/disk/by-uuid'
provide symlinks for each disk related volume label and serial
number:
'VOLUME_LABEL' -> '../../sdXN'
'VOLUME_SERIAL' -> '../../sdXN'

Signed-off-by: Christian Franke <christian.franke@t-online.de>
---
 winsup/cygwin/fhandler/dev_disk.cc      | 159 +++++++++++++++++++++---
 winsup/cygwin/local_includes/fhandler.h |   3 +-
 2 files changed, 141 insertions(+), 21 deletions(-)

diff --git a/winsup/cygwin/fhandler/dev_disk.cc b/winsup/cygwin/fhandler/dev_disk.cc
index 5f79ab5e9..016b4c7bc 100644
--- a/winsup/cygwin/fhandler/dev_disk.cc
+++ b/winsup/cygwin/fhandler/dev_disk.cc
@@ -14,33 +14,62 @@ details. */
 #include <wctype.h>
 #include <winioctl.h>
 
-/* Replace spaces, non-printing and unexpected characters.  Remove
-   leading and trailing spaces.  Return remaining string length. */
+/* Replace invalid characters.  Optionally remove leading and trailing
+   characters.  Return remaining string length. */
+template <typename char_type, typename func_type>
 static int
-sanitize_id_string (char *s)
+sanitize_string (char_type *s, char_type leading, char_type trailing,
+		 char_type replace, func_type valid)
 {
   int first = 0;
-  while (s[first] == ' ')
-    first++;
-  int last = -1, i;
+  if (leading)
+    while (s[first] == leading)
+      first++;
+  int len = -1, i;
   for (i = 0; s[first + i]; i++)
     {
-      char c = s[first + i];
-      if (c != ' ')
-	last = -1;
-      else if (last < 0)
-	last = i;
-      if (!(('0' <= c && c <= '9') || c == '.' || c == '-'
-	  || ('A' <= c && c <= 'Z') || ('a' <= c && c <= 'z')))
-	c = '_';
+      char_type c = s[first + i];
+      if (c != trailing)
+	len = -1;
+      else if (len < 0)
+	len = i;
+      if (!valid (c))
+	c = replace;
       else if (!first)
 	continue;
       s[i] = c;
     }
-  if (last < 0)
-    last = i;
-  s[last] = '\0';
-  return last;
+  if (len < 0)
+    len = i;
+  s[len] = (char_type) 0;
+  return len;
+}
+
+/* Variant for device identify strings. */
+static int
+sanitize_id_string (char *s)
+{
+  return sanitize_string (s, ' ', ' ', '_', [] (char c) -> bool
+    {
+      return (('0' <= c && c <= '9') || c == '.' || c == '-'
+	      || ('A' <= c && c <= 'Z') || ('a' <= c && c <= 'z'));
+    }
+  );
+}
+
+/* Variant for volume labels. */
+static int
+sanitize_label_string (WCHAR *s)
+{
+  /* Linux does not skip leading spaces. */
+  return sanitize_string (s, L'\0', L' ', L'_', [] (WCHAR c) -> bool
+    {
+      /* Labels may contain characters not allowed in filenames.
+	 Linux replaces spaces with \x20 which is not an option here. */
+      return !((0 <= c && c <= L' ') || c == L':' || c == L'/' || c == L'\\'
+	      || c == L'"');
+    }
+  );
 }
 
 /* Fetch storage properties and create the ID string.
@@ -244,6 +273,79 @@ partition_to_voluuid(const UNICODE_STRING *drive_uname, DWORD part_num,
   return true;
 }
 
+/* ("HarddiskN", PART_NUM) -> "VOLUME_LABEL" or "VOLUME_SERIAL" */
+static bool
+partition_to_label_or_uuid(bool uuid, const UNICODE_STRING *drive_uname,
+			   DWORD part_num, char *ioctl_buf, char *name)
+{
+  WCHAR wpath[MAX_PATH];
+  /* Trailing backslash is required. */
+  size_t len = __small_swprintf (wpath, L"\\Device\\%S\\Partition%u\\",
+				 drive_uname, part_num);
+  len *= sizeof (WCHAR);
+  UNICODE_STRING upath = {(USHORT) len, (USHORT) (len + 1), wpath};
+  OBJECT_ATTRIBUTES attr;
+  InitializeObjectAttributes (&attr, &upath, OBJ_CASE_INSENSITIVE, nullptr,
+			      nullptr);
+  IO_STATUS_BLOCK io;
+  HANDLE volhdl;
+  NTSTATUS status = NtOpenFile (&volhdl, READ_CONTROL, &attr, &io,
+				FILE_SHARE_VALID_FLAGS, 0);
+  if (!NT_SUCCESS (status))
+    {
+      /* Fails with STATUS_UNRECOGNIZED_VOLUME (0xC000014F) if the
+         partition/filesystem type is unsupported. */
+      debug_printf ("NtOpenFile(%S), status %y", upath, status);
+      return false;
+    }
+
+  /* Check for possible 64-bit NTFS serial number first. */
+  DWORD bytes_read;
+  const NTFS_VOLUME_DATA_BUFFER *nvdb =
+    reinterpret_cast<const NTFS_VOLUME_DATA_BUFFER *>(ioctl_buf);
+  if (uuid && DeviceIoControl (volhdl, FSCTL_GET_NTFS_VOLUME_DATA, nullptr, 0,
+			       ioctl_buf, NT_MAX_PATH, &bytes_read, nullptr)
+      && nvdb->VolumeSerialNumber.QuadPart)
+    {
+      /* Print without any separator as on Linux. */
+      __small_sprintf (name, "%16X", nvdb->VolumeSerialNumber.QuadPart);
+      NtClose(volhdl);
+      return true;
+    }
+
+  /* Get label and 32-bit serial number. */
+  status = NtQueryVolumeInformationFile (volhdl, &io, ioctl_buf,
+					 NT_MAX_PATH, FileFsVolumeInformation);
+  NtClose(volhdl);
+  if (!NT_SUCCESS (status))
+    {
+      debug_printf ("NtQueryVolumeInformationFile(%S), status %y", upath,
+		    status);
+      return false;
+    }
+
+  FILE_FS_VOLUME_INFORMATION *ffvi =
+    reinterpret_cast<FILE_FS_VOLUME_INFORMATION *>(ioctl_buf);
+  if (uuid)
+    {
+      if (!ffvi->VolumeSerialNumber)
+	return false;
+      /* Print with separator as on Linux. */
+      __small_sprintf (name, "%04x-%04x", ffvi->VolumeSerialNumber >> 16,
+		       ffvi->VolumeSerialNumber & 0xffff);
+    }
+  else
+    {
+      /* Label is not null terminated. */
+      ffvi->VolumeLabel[ffvi->VolumeLabelLength / sizeof(WCHAR)] = L'\0';
+      int len = sanitize_label_string (ffvi->VolumeLabel);
+      if (!len)
+	return false;
+      __small_sprintf (name, "%W", ffvi->VolumeLabel);
+    }
+  return true;
+}
+
 struct by_id_entry
 {
   char name[NAME_MAX + 1];
@@ -324,7 +426,11 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
   unsigned alloc_size = 0, table_size = 0;
   tmp_pathbuf tp;
   char *ioctl_buf = tp.c_get ();
-  WCHAR *w_buf = tp.w_get ();
+  char *ioctl_buf2 = (loc == fhandler_dev_disk::disk_by_label
+		      || loc == fhandler_dev_disk::disk_by_uuid ?
+		      tp.c_get () : nullptr);
+  WCHAR *w_buf = (loc == fhandler_dev_disk::disk_by_drive ?
+		  tp.w_get () : nullptr);
   DIRECTORY_BASIC_INFORMATION *dbi_buf =
     reinterpret_cast<DIRECTORY_BASIC_INFORMATION *>(tp.w_get ());
 
@@ -462,11 +568,23 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
 		    __small_sprintf (name, "%s-part%u", drive_name, part_num);
 		    break;
 
+		  case fhandler_dev_disk::disk_by_label:
+		    if (!partition_to_label_or_uuid (false, &dbi->ObjectName,
+						     part_num, ioctl_buf2, name))
+		      continue;
+		    break;
+
 		  case fhandler_dev_disk::disk_by_partuuid:
 		    if (!format_partuuid (name, pix))
 		      continue;
 		    break;
 
+		  case fhandler_dev_disk::disk_by_uuid:
+		    if (!partition_to_label_or_uuid (true, &dbi->ObjectName,
+						     part_num, ioctl_buf2, name))
+		      continue;
+		    break;
+
 		  case fhandler_dev_disk::disk_by_voluuid:
 		    if (!partition_to_voluuid (&dbi->ObjectName, part_num, name))
 		      continue;
@@ -521,7 +639,8 @@ const size_t by_drive_len = sizeof(by_drive) - 1;
 /* Keep this in sync with enum fhandler_dev_disk::dev_disk_location starting
    at disk_by_drive. */
 static const char * const by_dir_names[] {
-  "/by-drive", "/by-id", "/by-partuuid", "/by-voluuid"
+  "/by-drive", "/by-id", "/by-label",
+  "/by-partuuid", "/by-uuid", "/by-voluuid"
 };
 const size_t by_dir_names_size = sizeof(by_dir_names) / sizeof(by_dir_names[0]);
 static_assert((size_t) fhandler_dev_disk::disk_by_drive + by_dir_names_size - 1
diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/local_includes/fhandler.h
index 86c7b20a2..ca685a627 100644
--- a/winsup/cygwin/local_includes/fhandler.h
+++ b/winsup/cygwin/local_includes/fhandler.h
@@ -3198,7 +3198,8 @@ public:
   enum dev_disk_location {
     unknown_loc, invalid_loc, disk_dir,
     /* Keep these in sync with dev_disk.cc:by_dir_names array: */
-    disk_by_drive, disk_by_id, disk_by_partuuid, disk_by_voluuid
+    disk_by_drive, disk_by_id, disk_by_label,
+    disk_by_partuuid, disk_by_uuid, disk_by_voluuid
   };
 
 private:
-- 
2.42.1

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Corinna Vinschen]

On Nov 17 15:39, Christian Franke wrote:
> The last two /dev/disk subdirectories :-)
> 
> Note a minor difference: On Linux, empty /dev/disk subdirectories apparently
> never appear. A subdirectory is not listed in /dev/disk if it would be
> empty. Not worth the effort to emulate.

Agreed.  This is really great.  I just pushed your patch.

However, there's something strange in terms of by-label:

I have two partitions with labels:

  $ ls -l /dev/disk/by-label
  total 0
  lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub -> ../../sda3
  lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub2 -> ../../sdb2
  $

Now I change the label of sdb2 to the same "blub" string as on sda3:

  $ ls -l /dev/disk/by-label
  total 0
  $

I'd expected to see only one, due to the name collision, but en empty
dir is a bit surprising...  And it may occur more often than not, given
that the default label "New_Volume" probably won't get changed very
often.


Thanks,
Corinna

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Christian Franke]

Corinna Vinschen wrote:
> On Nov 17 15:39, Christian Franke wrote:
>> The last two /dev/disk subdirectories :-)
>>
>> Note a minor difference: On Linux, empty /dev/disk subdirectories apparently
>> never appear. A subdirectory is not listed in /dev/disk if it would be
>> empty. Not worth the effort to emulate.
> Agreed.  This is really great.  I just pushed your patch.
>
> However, there's something strange in terms of by-label:
>
> I have two partitions with labels:
>
>    $ ls -l /dev/disk/by-label
>    total 0
>    lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub -> ../../sda3
>    lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub2 -> ../../sdb2
>    $
>
> Now I change the label of sdb2 to the same "blub" string as on sda3:
>
>    $ ls -l /dev/disk/by-label
>    total 0
>    $
>
> I'd expected to see only one, due to the name collision, but en empty
> dir is a bit surprising...  And it may occur more often than not, given
> that the default label "New_Volume" probably won't get changed very
> often.
>

This is intentional and inherited from the very first patch, see the 
loop behind qsort(). If a range of identical names appear, all these 
entries are removed. If some "random" entry would be kept, it might no 
longer be the persistent link the user expects. We could possibly add 
some hash like done for by-id or append a number in such cases later. 
Need some more time to thing about it....

I will sent a patch for the new-features doc soon.

Christian

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Corinna Vinschen]

On Nov 17 17:45, Christian Franke wrote:
> Corinna Vinschen wrote:
> > On Nov 17 15:39, Christian Franke wrote:
> > > The last two /dev/disk subdirectories :-)
> > > 
> > > Note a minor difference: On Linux, empty /dev/disk subdirectories apparently
> > > never appear. A subdirectory is not listed in /dev/disk if it would be
> > > empty. Not worth the effort to emulate.
> > Agreed.  This is really great.  I just pushed your patch.
> > 
> > However, there's something strange in terms of by-label:
> > 
> > I have two partitions with labels:
> > 
> >    $ ls -l /dev/disk/by-label
> >    total 0
> >    lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub -> ../../sda3
> >    lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub2 -> ../../sdb2
> >    $
> > 
> > Now I change the label of sdb2 to the same "blub" string as on sda3:
> > 
> >    $ ls -l /dev/disk/by-label
> >    total 0
> >    $
> > 
> > I'd expected to see only one, due to the name collision, but en empty
> > dir is a bit surprising...  And it may occur more often than not, given
> > that the default label "New_Volume" probably won't get changed very
> > often.
> > 
> 
> This is intentional and inherited from the very first patch, see the loop
> behind qsort(). If a range of identical names appear, all these entries are
> removed. If some "random" entry would be kept, it might no longer be the
> persistent link the user expects. We could possibly add some hash like done
> for by-id or append a number in such cases later. Need some more time to
> thing about it....

I see.  Admittedly, I don't know how Linux handles this either.

> I will sent a patch for the new-features doc soon.

Thanks,
Corinna

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 1937 bytes --]

Corinna Vinschen wrote:
> On Nov 17 17:45, Christian Franke wrote:
>> Corinna Vinschen wrote:
>>> On Nov 17 15:39, Christian Franke wrote:
>>>> The last two /dev/disk subdirectories :-)
>>>>
>>>> Note a minor difference: On Linux, empty /dev/disk subdirectories apparently
>>>> never appear. A subdirectory is not listed in /dev/disk if it would be
>>>> empty. Not worth the effort to emulate.
>>> Agreed.  This is really great.  I just pushed your patch.
>>>
>>> However, there's something strange in terms of by-label:
>>>
>>> I have two partitions with labels:
>>>
>>>     $ ls -l /dev/disk/by-label
>>>     total 0
>>>     lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub -> ../../sda3
>>>     lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub2 -> ../../sdb2
>>>     $
>>>
>>> Now I change the label of sdb2 to the same "blub" string as on sda3:
>>>
>>>     $ ls -l /dev/disk/by-label
>>>     total 0
>>>     $
>>>
>>> I'd expected to see only one, due to the name collision, but en empty
>>> dir is a bit surprising...  And it may occur more often than not, given
>>> that the default label "New_Volume" probably won't get changed very
>>> often.
>>>
>> This is intentional and inherited from the very first patch, see the loop
>> behind qsort(). If a range of identical names appear, all these entries are
>> removed. If some "random" entry would be kept, it might no longer be the
>> persistent link the user expects. We could possibly add some hash like done
>> for by-id or append a number in such cases later. Need some more time to
>> thing about it....
> I see.  Admittedly, I don't know how Linux handles this either.

A quick test on Debian 12 with by-label suggests that the last duplicate 
wins. Also not very sophisticated :-)
IIRC in the past I've seen in another of these directories (by-id?) that 
'#N' was appended if duplicates occur.


>> I will sent a patch for the new-features doc soon.

Attached.

Christian


[-- Attachment #2: 0001-Cygwin-Document-dev-disk-by-subdirectories.patch --]
[-- Type: text/plain, Size: 3140 bytes --]

From 5d1c82f08e4b37d65acc8bf0c76f75641d762667 Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Fri, 17 Nov 2023 18:41:08 +0100
Subject: [PATCH] Cygwin: Document /dev/disk/by-* subdirectories

Signed-off-by: Christian Franke <christian.franke@t-online.de>
---
 winsup/cygwin/release/3.5.0 | 16 +++++++++++-----
 winsup/doc/new-features.xml | 20 +++++++++++---------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/winsup/cygwin/release/3.5.0 b/winsup/cygwin/release/3.5.0
index 2d59818b5..aee21c972 100644
--- a/winsup/cygwin/release/3.5.0
+++ b/winsup/cygwin/release/3.5.0
@@ -17,11 +17,17 @@ What's new:
   class expressions, and collating symbols in the search pattern, i.e.,
   [:alnum:], [=a=], [.aa.].
 
-- Introduce /dev/disk directory with subdirectories by-id and by-partuuid.
-  The by-id directory provides symlinks for each disk and its partitions:
-  BUSTYPE-[VENDOR_]PRODUCT_[SERIAL|HASH][-partN] -> ../../sdX[N].
-  The by-partuuid directory provides symlinks for each MBR and GPT disk
-  partition: MBR_SERIAL-OFFSET -> ../../sdXN, GPT_GUID -> ../../sdXN.
+- Introduce /dev/disk directory with various by-* subdirectories which
+  provide symlinks to disk and partition raw devices:
+  by-drive/DRIVE_LETTER ->  ../../sdXN
+  by-label/VOLUME_LABEL ->  ../../sdXN
+  by-id/BUSTYPE-[VENDOR_]PRODUCT_[SERIAL|0xHASH][-partN] -> ../../sdX[N]
+  by-partuuid/MBR_SERIAL-OFFSET -> ../../sdXN
+  by-partuuid/GPT_GUID -> ../../sdXN
+  by-uuid/VOLUME_SERIAL -> ../../sdXN
+  by-voluuid/MBR_SERIAL-OFFSET -> ../../sdXN
+  by-voluuid/VOLUME_GUID -> ../../sdXN
+  The subdirectories by-drive and by-voluuid are Cygwin specific.
 
 - Introduce /proc/codesets and /proc/locales with information on
   supported codesets and locales for all interested parties.  Locale(1)
diff --git a/winsup/doc/new-features.xml b/winsup/doc/new-features.xml
index a8e8a7991..2c31a4acc 100644
--- a/winsup/doc/new-features.xml
+++ b/winsup/doc/new-features.xml
@@ -35,17 +35,19 @@ class expressions, and collating symbols in the search pattern, i.e.,
 </para></listitem>
 
 <listitem><para>
-Introduce /dev/disk directory with subdirectories by-id and by-partuuid.
-The by-id directory provides symlinks for each disk and its partitions:
+Introduce /dev/disk directory with various by-* subdirectories which
+provide symlinks to disk and partition raw devices:
   <screen>
-  BUSTYPE-[VENDOR_]PRODUCT_[SERIAL|0xHASH][-partN] -> ../../sdX[N]
-  </screen>
-The by-partuuid directory provides symlinks for each MBR and GPT disk
-partition:
-  <screen>
-  MBR_SERIAL-OFFSET -> ../../sdXN
-  GPT_GUID -> ../../sdXN
+  by-drive/DRIVE_LETTER ->  ../../sdXN
+  by-label/VOLUME_LABEL ->  ../../sdXN
+  by-id/BUSTYPE-[VENDOR_]PRODUCT_[SERIAL|0xHASH][-partN] -> ../../sdX[N]
+  by-partuuid/MBR_SERIAL-OFFSET -> ../../sdXN
+  by-partuuid/GPT_GUID -> ../../sdXN
+  by-uuid/VOLUME_SERIAL -> ../../sdXN
+  by-voluuid/MBR_SERIAL-OFFSET -> ../../sdXN
+  by-voluuid/VOLUME_GUID -> ../../sdXN
   </screen>
+The subdirectories by-drive and by-voluuid are Cygwin specific.
 </para></listitem>
 
 <listitem><para>
-- 
2.42.1

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Corinna Vinschen]

On Nov 17 18:53, Christian Franke wrote:
> Corinna Vinschen wrote:
> > On Nov 17 17:45, Christian Franke wrote:
> > > Corinna Vinschen wrote:
> > > > On Nov 17 15:39, Christian Franke wrote:
> > > > > The last two /dev/disk subdirectories :-)
> > > > > 
> > > > > Note a minor difference: On Linux, empty /dev/disk subdirectories apparently
> > > > > never appear. A subdirectory is not listed in /dev/disk if it would be
> > > > > empty. Not worth the effort to emulate.
> > > > Agreed.  This is really great.  I just pushed your patch.
> > > > 
> > > > However, there's something strange in terms of by-label:
> > > > 
> > > > I have two partitions with labels:
> > > > 
> > > >     $ ls -l /dev/disk/by-label
> > > >     total 0
> > > >     lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub -> ../../sda3
> > > >     lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub2 -> ../../sdb2
> > > >     $
> > > > 
> > > > Now I change the label of sdb2 to the same "blub" string as on sda3:
> > > > 
> > > >     $ ls -l /dev/disk/by-label
> > > >     total 0
> > > >     $
> > > > 
> > > > I'd expected to see only one, due to the name collision, but en empty
> > > > dir is a bit surprising...  And it may occur more often than not, given
> > > > that the default label "New_Volume" probably won't get changed very
> > > > often.
> > > > 
> > > This is intentional and inherited from the very first patch, see the loop
> > > behind qsort(). If a range of identical names appear, all these entries are
> > > removed. If some "random" entry would be kept, it might no longer be the
> > > persistent link the user expects. We could possibly add some hash like done
> > > for by-id or append a number in such cases later. Need some more time to
> > > thing about it....
> > I see.  Admittedly, I don't know how Linux handles this either.
> 
> A quick test on Debian 12 with by-label suggests that the last duplicate
> wins. Also not very sophisticated :-)

Given this is all controlled by rather simple udev rules, see
/usr/lib/udev/rules.d/60-persistent-storage.rules, that's not
really surprising.

> IIRC in the past I've seen in another of these directories (by-id?) that
> '#N' was appended if duplicates occur.

I don't see anything like that in 60-persistent-storage.rules, though.
It has been removed at one point, it seems.

> > > I will sent a patch for the new-features doc soon.
> 
> Attached.

Thanks, pushed.


Corinna

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

Corinna Vinschen wrote:
> On Nov 17 18:53, Christian Franke wrote:
>> Corinna Vinschen wrote:
>>> ...
>>> I see.  Admittedly, I don't know how Linux handles this either.
>> A quick test on Debian 12 with by-label suggests that the last duplicate
>> wins. Also not very sophisticated :-)
> Given this is all controlled by rather simple udev rules, see
> /usr/lib/udev/rules.d/60-persistent-storage.rules, that's not
> really surprising.
>
>> IIRC in the past I've seen in another of these directories (by-id?) that
>> '#N' was appended if duplicates occur.
> I don't see anything like that in 60-persistent-storage.rules, though.
> It has been removed at one point, it seems.

A quick followup to mark duplicates and (more important) avoid leading 
spaces in NTFS serial number.


[-- Attachment #2: 0001-Cygwin-dev-disk-Mark-duplicates-fix-serial-number-fo.patch --]
[-- Type: text/plain, Size: 4092 bytes --]

From 1e212814cfb17579105ead4835410bd74dfb188a Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Fri, 17 Nov 2023 21:20:26 +0100
Subject: [PATCH] Cygwin: /dev/disk: Mark duplicates, fix serial number format

Keep ranges of duplicate names and append '#N' to each name.
Add missing leading zeros to output format of NTFS serial number.
No longer ignore null volume serial numbers.

Signed-off-by: Christian Franke <christian.franke@t-online.de>
---
 winsup/cygwin/fhandler/dev_disk.cc | 38 ++++++++++++++----------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/winsup/cygwin/fhandler/dev_disk.cc b/winsup/cygwin/fhandler/dev_disk.cc
index 016b4c7bc..131ffd935 100644
--- a/winsup/cygwin/fhandler/dev_disk.cc
+++ b/winsup/cygwin/fhandler/dev_disk.cc
@@ -64,10 +64,12 @@ sanitize_label_string (WCHAR *s)
   /* Linux does not skip leading spaces. */
   return sanitize_string (s, L'\0', L' ', L'_', [] (WCHAR c) -> bool
     {
-      /* Labels may contain characters not allowed in filenames.
-	 Linux replaces spaces with \x20 which is not an option here. */
+      /* Labels may contain characters not allowed in filenames.  Also
+         replace '#' to avoid that duplicate markers introduce new
+	 duplicates.  Linux replaces spaces with \x20 which is not an
+	 option here. */
       return !((0 <= c && c <= L' ') || c == L':' || c == L'/' || c == L'\\'
-	      || c == L'"');
+	      || c == L'#' || c == L'"');
     }
   );
 }
@@ -304,11 +306,10 @@ partition_to_label_or_uuid(bool uuid, const UNICODE_STRING *drive_uname,
   const NTFS_VOLUME_DATA_BUFFER *nvdb =
     reinterpret_cast<const NTFS_VOLUME_DATA_BUFFER *>(ioctl_buf);
   if (uuid && DeviceIoControl (volhdl, FSCTL_GET_NTFS_VOLUME_DATA, nullptr, 0,
-			       ioctl_buf, NT_MAX_PATH, &bytes_read, nullptr)
-      && nvdb->VolumeSerialNumber.QuadPart)
+			       ioctl_buf, NT_MAX_PATH, &bytes_read, nullptr))
     {
       /* Print without any separator as on Linux. */
-      __small_sprintf (name, "%16X", nvdb->VolumeSerialNumber.QuadPart);
+      __small_sprintf (name, "%016X", nvdb->VolumeSerialNumber.QuadPart);
       NtClose(volhdl);
       return true;
     }
@@ -327,13 +328,9 @@ partition_to_label_or_uuid(bool uuid, const UNICODE_STRING *drive_uname,
   FILE_FS_VOLUME_INFORMATION *ffvi =
     reinterpret_cast<FILE_FS_VOLUME_INFORMATION *>(ioctl_buf);
   if (uuid)
-    {
-      if (!ffvi->VolumeSerialNumber)
-	return false;
-      /* Print with separator as on Linux. */
-      __small_sprintf (name, "%04x-%04x", ffvi->VolumeSerialNumber >> 16,
-		       ffvi->VolumeSerialNumber & 0xffff);
-    }
+    /* Print with separator as on Linux. */
+    __small_sprintf (name, "%04x-%04x", ffvi->VolumeSerialNumber >> 16,
+		     ffvi->VolumeSerialNumber & 0xffff);
   else
     {
       /* Label is not null terminated. */
@@ -610,7 +607,7 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
   if (!table)
     return (errno_set ? -1 : 0);
 
-  /* Sort by name and remove duplicates. */
+  /* Sort by name and mark duplicates. */
   qsort (table, table_size, sizeof (*table), by_id_compare_name);
   for (unsigned i = 0; i < table_size; i++)
     {
@@ -619,12 +616,13 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
 	j++;
       if (j == i + 1)
 	continue;
-      /* Duplicate(s) found, remove all entries with this name. */
-      debug_printf ("removing duplicates %d-%d: '%s'", i, j - 1, table[i].name);
-      if (j < table_size)
-	memmove (table + i, table + j, (table_size - j) * sizeof (*table));
-      table_size -= j - i;
-      i--;
+      /* Duplicate(s) found, append "#N" to all entries.  This never
+	 introduces new duplicates because '#' never occurs in the
+	 original names. */
+      debug_printf ("mark duplicates %u-%u of '%s'", i, j - 1, table[i].name);
+      size_t len = strlen (table[i].name);
+      for (unsigned k = i; k < j; k++)
+	__small_sprintf (table[k].name + len, "#%u", k - i);
     }
 
   debug_printf ("table_size: %d", table_size);
-- 
2.42.1

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Corinna Vinschen]

Hi Christian,

This puzzles me:

On Nov 17 21:25, Christian Franke wrote:
> @@ -610,7 +607,7 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
>    if (!table)
>      return (errno_set ? -1 : 0);
>  
> -  /* Sort by name and remove duplicates. */
> +  /* Sort by name and mark duplicates. */
>    qsort (table, table_size, sizeof (*table), by_id_compare_name);
>    for (unsigned i = 0; i < table_size; i++)

by_id_compare_name only compars the actual names...

>      {
> @@ -619,12 +616,13 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
>  	j++;
>        if (j == i + 1)
>  	continue;
> -      /* Duplicate(s) found, remove all entries with this name. */
> -      debug_printf ("removing duplicates %d-%d: '%s'", i, j - 1, table[i].name);
> -      if (j < table_size)
> -	memmove (table + i, table + j, (table_size - j) * sizeof (*table));
> -      table_size -= j - i;
> -      i--;
> +      /* Duplicate(s) found, append "#N" to all entries.  This never

...but the names are identical.  So the *order* within the identically
named entries depends on qsort's reshuffling of table
entries.  Which in turn depends on outside factors like number of table
entries and the ultimate position of the identical entries within the
ordered table.

Having said that, I don't see how adding ordinals to the names can be
unambiguous.  AFAICS, the numbers may change by just adding another
disk (USB Stick) to the system...


Thanks,
Corinna

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Corinna Vinschen]

On Nov 20 10:40, Corinna Vinschen wrote:
> Hi Christian,
> 
> This puzzles me:
> 
> On Nov 17 21:25, Christian Franke wrote:
> > @@ -610,7 +607,7 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
> >    if (!table)
> >      return (errno_set ? -1 : 0);
> >  
> > -  /* Sort by name and remove duplicates. */
> > +  /* Sort by name and mark duplicates. */
> >    qsort (table, table_size, sizeof (*table), by_id_compare_name);
> >    for (unsigned i = 0; i < table_size; i++)
> 
> by_id_compare_name only compars the actual names...
> 
> >      {
> > @@ -619,12 +616,13 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
> >  	j++;
> >        if (j == i + 1)
> >  	continue;
> > -      /* Duplicate(s) found, remove all entries with this name. */
> > -      debug_printf ("removing duplicates %d-%d: '%s'", i, j - 1, table[i].name);
> > -      if (j < table_size)
> > -	memmove (table + i, table + j, (table_size - j) * sizeof (*table));
> > -      table_size -= j - i;
> > -      i--;
> > +      /* Duplicate(s) found, append "#N" to all entries.  This never
> 
> ...but the names are identical.  So the *order* within the identically
> named entries depends on qsort's reshuffling of table
> entries.  Which in turn depends on outside factors like number of table
> entries and the ultimate position of the identical entries within the
> ordered table.
> 
> Having said that, I don't see how adding ordinals to the names can be
> unambiguous.  AFAICS, the numbers may change by just adding another
> disk (USB Stick) to the system...

Oops, that's not exactly what I was trying to say, sorry.

The problem is not adding ordinals to the name, AFAICS, the problem is
that the sorting function by_id_compare_name is not up to the task to
make sure the order is unambiguous within the entries of identical name.


Corinna

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]

Corinna Vinschen wrote:
> On Nov 20 10:40, Corinna Vinschen wrote:
>> Hi Christian,
>>
>> This puzzles me:
>>
>> On Nov 17 21:25, Christian Franke wrote:
>>> @@ -610,7 +607,7 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
>>>     if (!table)
>>>       return (errno_set ? -1 : 0);
>>>   
>>> -  /* Sort by name and remove duplicates. */
>>> +  /* Sort by name and mark duplicates. */
>>>     qsort (table, table_size, sizeof (*table), by_id_compare_name);
>>>     for (unsigned i = 0; i < table_size; i++)
>> by_id_compare_name only compars the actual names...
>>
>>>       {
>>> @@ -619,12 +616,13 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
>>>   	j++;
>>>         if (j == i + 1)
>>>   	continue;
>>> -      /* Duplicate(s) found, remove all entries with this name. */
>>> -      debug_printf ("removing duplicates %d-%d: '%s'", i, j - 1, table[i].name);
>>> -      if (j < table_size)
>>> -	memmove (table + i, table + j, (table_size - j) * sizeof (*table));
>>> -      table_size -= j - i;
>>> -      i--;
>>> +      /* Duplicate(s) found, append "#N" to all entries.  This never
>> ...but the names are identical.  So the *order* within the identically
>> named entries depends on qsort's reshuffling of table
>> entries.  Which in turn depends on outside factors like number of table
>> entries and the ultimate position of the identical entries within the
>> ordered table.
>>
>> Having said that, I don't see how adding ordinals to the names can be
>> unambiguous.  AFAICS, the numbers may change by just adding another
>> disk (USB Stick) to the system...
> Oops, that's not exactly what I was trying to say, sorry.
>
> The problem is not adding ordinals to the name, AFAICS, the problem is
> that the sorting function by_id_compare_name is not up to the task to
> make sure the order is unambiguous within the entries of identical name.

That's correct, thanks for catching. qsort is not a stable sort. 
Changing drives outside the duplicate range may also change the order 
within the range. Could be fixed by a lexicographic compare of {name, 
drive, part}.

I'll provide a new patch soon. For now, I attached the unrelated but 
important part of the dropped patch.

Christian


[-- Attachment #2: 0001-Cygwin-dev-disk-by-uuid-Fix-NTFS-serial-number-print.patch --]
[-- Type: text/plain, Size: 1015 bytes --]

From 64c8a0eb3ead91de36ee65516483e40cf7f49446 Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Mon, 20 Nov 2023 15:40:42 +0100
Subject: [PATCH] Cygwin: /dev/disk/by-uuid: Fix NTFS serial number print
 format

Signed-off-by: Christian Franke <christian.franke@t-online.de>
---
 winsup/cygwin/fhandler/dev_disk.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/winsup/cygwin/fhandler/dev_disk.cc b/winsup/cygwin/fhandler/dev_disk.cc
index 016b4c7bc..c5d72816f 100644
--- a/winsup/cygwin/fhandler/dev_disk.cc
+++ b/winsup/cygwin/fhandler/dev_disk.cc
@@ -308,7 +308,7 @@ partition_to_label_or_uuid(bool uuid, const UNICODE_STRING *drive_uname,
       && nvdb->VolumeSerialNumber.QuadPart)
     {
       /* Print without any separator as on Linux. */
-      __small_sprintf (name, "%16X", nvdb->VolumeSerialNumber.QuadPart);
+      __small_sprintf (name, "%016X", nvdb->VolumeSerialNumber.QuadPart);
       NtClose(volhdl);
       return true;
     }
-- 
2.42.1

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Corinna Vinschen]

On Nov 20 15:54, Christian Franke wrote:
> From: Christian Franke <christian.franke@t-online.de>
> Date: Mon, 20 Nov 2023 15:40:42 +0100
> Subject: [PATCH] Cygwin: /dev/disk/by-uuid: Fix NTFS serial number print
>  format
> 
> Signed-off-by: Christian Franke <christian.franke@t-online.de>
> ---
>  winsup/cygwin/fhandler/dev_disk.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/winsup/cygwin/fhandler/dev_disk.cc b/winsup/cygwin/fhandler/dev_disk.cc
> index 016b4c7bc..c5d72816f 100644
> --- a/winsup/cygwin/fhandler/dev_disk.cc
> +++ b/winsup/cygwin/fhandler/dev_disk.cc
> @@ -308,7 +308,7 @@ partition_to_label_or_uuid(bool uuid, const UNICODE_STRING *drive_uname,
>        && nvdb->VolumeSerialNumber.QuadPart)
>      {
>        /* Print without any separator as on Linux. */
> -      __small_sprintf (name, "%16X", nvdb->VolumeSerialNumber.QuadPart);
> +      __small_sprintf (name, "%016X", nvdb->VolumeSerialNumber.QuadPart);
>        NtClose(volhdl);
>        return true;
>      }
> -- 
> 2.42.1
> 

Pushed.

Thanks,
Corinna

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]

Christian Franke wrote:
> Corinna Vinschen wrote:
>> On Nov 20 10:40, Corinna Vinschen wrote:
>>> Hi Christian,
>>>
>>> This puzzles me:
>>>
>>> On Nov 17 21:25, Christian Franke wrote:
>>>> @@ -610,7 +607,7 @@ get_by_id_table (by_id_entry * &table, 
>>>> fhandler_dev_disk::dev_disk_location loc)
>>>>     if (!table)
>>>>       return (errno_set ? -1 : 0);
>>>>   -  /* Sort by name and remove duplicates. */
>>>> +  /* Sort by name and mark duplicates. */
>>>>     qsort (table, table_size, sizeof (*table), by_id_compare_name);
>>>>     for (unsigned i = 0; i < table_size; i++)
>>> by_id_compare_name only compars the actual names...
>>>
>>>>       {
>>>> @@ -619,12 +616,13 @@ get_by_id_table (by_id_entry * &table, 
>>>> fhandler_dev_disk::dev_disk_location loc)
>>>>       j++;
>>>>         if (j == i + 1)
>>>>       continue;
>>>> -      /* Duplicate(s) found, remove all entries with this name. */
>>>> -      debug_printf ("removing duplicates %d-%d: '%s'", i, j - 1, 
>>>> table[i].name);
>>>> -      if (j < table_size)
>>>> -    memmove (table + i, table + j, (table_size - j) * sizeof 
>>>> (*table));
>>>> -      table_size -= j - i;
>>>> -      i--;
>>>> +      /* Duplicate(s) found, append "#N" to all entries. This never
>>> ...but the names are identical.  So the *order* within the identically
>>> named entries depends on qsort's reshuffling of table
>>> entries.  Which in turn depends on outside factors like number of table
>>> entries and the ultimate position of the identical entries within the
>>> ordered table.
>>>
>>> Having said that, I don't see how adding ordinals to the names can be
>>> unambiguous.  AFAICS, the numbers may change by just adding another
>>> disk (USB Stick) to the system...
>> Oops, that's not exactly what I was trying to say, sorry.
>>
>> The problem is not adding ordinals to the name, AFAICS, the problem is
>> that the sorting function by_id_compare_name is not up to the task to
>> make sure the order is unambiguous within the entries of identical name.
>
> That's correct, thanks for catching. qsort is not a stable sort. 
> Changing drives outside the duplicate range may also change the order 
> within the range. Could be fixed by a lexicographic compare of {name, 
> drive, part}.
>

Attached.


[-- Attachment #2: 0001-Cygwin-dev-disk-Append-N-if-the-same-name-appears-mo.patch --]
[-- Type: text/plain, Size: 4665 bytes --]

From 7dafb85210ef77ea8798f22160f7782c394ef5c3 Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Tue, 21 Nov 2023 12:17:14 +0100
Subject: [PATCH] Cygwin: /dev/disk: Append '#N' if the same name appears more
 than once

No longer drop ranges of identical link names.  Append '#0, #1, ...'
to each name instead.  No longer ignore null volume serial numbers.

Signed-off-by: Christian Franke <christian.franke@t-online.de>
---
 winsup/cygwin/fhandler/dev_disk.cc | 53 +++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/winsup/cygwin/fhandler/dev_disk.cc b/winsup/cygwin/fhandler/dev_disk.cc
index c5d72816f..d12ac52fa 100644
--- a/winsup/cygwin/fhandler/dev_disk.cc
+++ b/winsup/cygwin/fhandler/dev_disk.cc
@@ -64,10 +64,12 @@ sanitize_label_string (WCHAR *s)
   /* Linux does not skip leading spaces. */
   return sanitize_string (s, L'\0', L' ', L'_', [] (WCHAR c) -> bool
     {
-      /* Labels may contain characters not allowed in filenames.
-	 Linux replaces spaces with \x20 which is not an option here. */
+      /* Labels may contain characters not allowed in filenames.  Also
+         replace '#' to avoid that duplicate markers introduce new
+	 duplicates.  Linux replaces spaces with \x20 which is not an
+	 option here. */
       return !((0 <= c && c <= L' ') || c == L':' || c == L'/' || c == L'\\'
-	      || c == L'"');
+	      || c == L'#' || c == L'"');
     }
   );
 }
@@ -304,8 +306,7 @@ partition_to_label_or_uuid(bool uuid, const UNICODE_STRING *drive_uname,
   const NTFS_VOLUME_DATA_BUFFER *nvdb =
     reinterpret_cast<const NTFS_VOLUME_DATA_BUFFER *>(ioctl_buf);
   if (uuid && DeviceIoControl (volhdl, FSCTL_GET_NTFS_VOLUME_DATA, nullptr, 0,
-			       ioctl_buf, NT_MAX_PATH, &bytes_read, nullptr)
-      && nvdb->VolumeSerialNumber.QuadPart)
+			       ioctl_buf, NT_MAX_PATH, &bytes_read, nullptr))
     {
       /* Print without any separator as on Linux. */
       __small_sprintf (name, "%016X", nvdb->VolumeSerialNumber.QuadPart);
@@ -327,13 +328,9 @@ partition_to_label_or_uuid(bool uuid, const UNICODE_STRING *drive_uname,
   FILE_FS_VOLUME_INFORMATION *ffvi =
     reinterpret_cast<FILE_FS_VOLUME_INFORMATION *>(ioctl_buf);
   if (uuid)
-    {
-      if (!ffvi->VolumeSerialNumber)
-	return false;
-      /* Print with separator as on Linux. */
-      __small_sprintf (name, "%04x-%04x", ffvi->VolumeSerialNumber >> 16,
-		       ffvi->VolumeSerialNumber & 0xffff);
-    }
+    /* Print with separator as on Linux. */
+    __small_sprintf (name, "%04x-%04x", ffvi->VolumeSerialNumber >> 16,
+		     ffvi->VolumeSerialNumber & 0xffff);
   else
     {
       /* Label is not null terminated. */
@@ -361,6 +358,20 @@ by_id_compare_name (const void *a, const void *b)
   return strcmp (ap->name, bp->name);
 }
 
+static int
+by_id_compare_name_drive_part (const void *a, const void *b)
+{
+  const by_id_entry *ap = reinterpret_cast<const by_id_entry *>(a);
+  const by_id_entry *bp = reinterpret_cast<const by_id_entry *>(b);
+  int cmp = strcmp (ap->name, bp->name);
+  if (cmp)
+    return cmp;
+  cmp = ap->drive - bp->drive;
+  if (cmp)
+    return cmp;
+  return ap->part - bp->part;
+}
+
 static by_id_entry *
 by_id_realloc (by_id_entry *p, size_t n)
 {
@@ -610,8 +621,9 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
   if (!table)
     return (errno_set ? -1 : 0);
 
-  /* Sort by name and remove duplicates. */
-  qsort (table, table_size, sizeof (*table), by_id_compare_name);
+  /* Sort by {name, drive, part} to ensure stable sort order. */
+  qsort (table, table_size, sizeof (*table), by_id_compare_name_drive_part);
+  /* Mark duplicate names. */
   for (unsigned i = 0; i < table_size; i++)
     {
       unsigned j = i + 1;
@@ -619,12 +631,13 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
 	j++;
       if (j == i + 1)
 	continue;
-      /* Duplicate(s) found, remove all entries with this name. */
-      debug_printf ("removing duplicates %d-%d: '%s'", i, j - 1, table[i].name);
-      if (j < table_size)
-	memmove (table + i, table + j, (table_size - j) * sizeof (*table));
-      table_size -= j - i;
-      i--;
+      /* Duplicate(s) found, append "#N" to all entries.  This never
+	 introduces new duplicates because '#' never occurs in the
+	 original names. */
+      debug_printf ("mark duplicates %u-%u of '%s'", i, j - 1, table[i].name);
+      size_t len = strlen (table[i].name);
+      for (unsigned k = i; k < j; k++)
+	__small_sprintf (table[k].name + len, "#%u", k - i);
     }
 
   debug_printf ("table_size: %d", table_size);
-- 
2.42.1

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Corinna Vinschen]

Hi Christian,

Looks good, but I just realized that I was already wondering about the
sanitization and forgot to talk about it:

On Nov 21 12:24, Christian Franke wrote:
> diff --git a/winsup/cygwin/fhandler/dev_disk.cc b/winsup/cygwin/fhandler/dev_disk.cc
> index c5d72816f..d12ac52fa 100644
> --- a/winsup/cygwin/fhandler/dev_disk.cc
> +++ b/winsup/cygwin/fhandler/dev_disk.cc
> @@ -64,10 +64,12 @@ sanitize_label_string (WCHAR *s)
>    /* Linux does not skip leading spaces. */
>    return sanitize_string (s, L'\0', L' ', L'_', [] (WCHAR c) -> bool
>      {
> -      /* Labels may contain characters not allowed in filenames.
> -	 Linux replaces spaces with \x20 which is not an option here. */
> +      /* Labels may contain characters not allowed in filenames.  Also

Apart from slash and backslash, we don't have this problem in Cygwin,
usually.  Even control characters are no problem.  All chars not allowed
in filenames are just transposed into the Unicode private use area, as
per strfuncs.cc, line 20ff on the way to storage, and back when reading
the names from storage.  This, and especially in a virtual filesystem
like /proc, there's no reason to avoid these characters.

> +         replace '#' to avoid that duplicate markers introduce new
> +	 duplicates.  Linux replaces spaces with \x20 which is not an
> +	 option here. */
>        return !((0 <= c && c <= L' ') || c == L':' || c == L'/' || c == L'\\'
> -	      || c == L'"');
> +	      || c == L'#' || c == L'"');

If you really want to avoid chars not allowed in DOS filenames, the
list seems incomplete, missing '<', '>', '?', '*', '|'.

But as I said, there's really no reason for that.  I simply reduced the
above expression to

  return !(c == L'/' || c == L'\\' || c == L'#');

and created a disk label

  test"foo*bar?baz:"

It works nicely, including stuff like

  $ ls *\"*
  $ ls *\**

So, I can push it as is, or we just allow everything and the kitchen sink
as per the reduced filter expression.  What do you prefer?


Thanks,
Corinna

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]

Corinna Vinschen wrote:
> Hi Christian,
>
> Looks good, but I just realized that I was already wondering about the
> sanitization and forgot to talk about it:
>
> On Nov 21 12:24, Christian Franke wrote:
>> diff --git a/winsup/cygwin/fhandler/dev_disk.cc b/winsup/cygwin/fhandler/dev_disk.cc
>> index c5d72816f..d12ac52fa 100644
>> --- a/winsup/cygwin/fhandler/dev_disk.cc
>> +++ b/winsup/cygwin/fhandler/dev_disk.cc
>> @@ -64,10 +64,12 @@ sanitize_label_string (WCHAR *s)
>>     /* Linux does not skip leading spaces. */
>>     return sanitize_string (s, L'\0', L' ', L'_', [] (WCHAR c) -> bool
>>       {
>> -      /* Labels may contain characters not allowed in filenames.
>> -	 Linux replaces spaces with \x20 which is not an option here. */
>> +      /* Labels may contain characters not allowed in filenames.  Also
> Apart from slash and backslash, we don't have this problem in Cygwin,
> usually.  Even control characters are no problem.  All chars not allowed
> in filenames are just transposed into the Unicode private use area, as
> per strfuncs.cc, line 20ff on the way to storage, and back when reading
> the names from storage.  This, and especially in a virtual filesystem
> like /proc, there's no reason to avoid these characters.

Thanks for clarification.


>
>> +         replace '#' to avoid that duplicate markers introduce new
>> +	 duplicates.  Linux replaces spaces with \x20 which is not an
>> +	 option here. */
>>         return !((0 <= c && c <= L' ') || c == L':' || c == L'/' || c == L'\\'
>> -	      || c == L'"');
>> +	      || c == L'#' || c == L'"');
> If you really want to avoid chars not allowed in DOS filenames, the
> list seems incomplete, missing '<', '>', '?', '*', '|'.
>
> But as I said, there's really no reason for that.  I simply reduced the
> above expression to
>
>    return !(c == L'/' || c == L'\\' || c == L'#');
>
> and created a disk label
>
>    test"foo*bar?baz:"
>
> It works nicely, including stuff like
>
>    $ ls *\"*
>    $ ls *\**
>
> So, I can push it as is, or we just allow everything and the kitchen sink
> as per the reduced filter expression.  What do you prefer?

The latter - patch attached.

Christian


[-- Attachment #2: 0001-Cygwin-dev-disk-Append-N-if-the-same-name-appears-mo.patch --]
[-- Type: text/plain, Size: 4724 bytes --]

From ecc54356adbe7768bd5fd5561c78c67cd5725183 Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Tue, 21 Nov 2023 19:28:02 +0100
Subject: [PATCH] Cygwin: /dev/disk: Append '#N' if the same name appears more
 than once

No longer drop ranges of identical link names.  Append '#0, #1, ...'
to each name instead.  Enhance charset allowed in label names.
No longer ignore null volume serial numbers.

Signed-off-by: Christian Franke <christian.franke@t-online.de>
---
 winsup/cygwin/fhandler/dev_disk.cc | 54 ++++++++++++++++++------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/winsup/cygwin/fhandler/dev_disk.cc b/winsup/cygwin/fhandler/dev_disk.cc
index c5d72816f..29af9de95 100644
--- a/winsup/cygwin/fhandler/dev_disk.cc
+++ b/winsup/cygwin/fhandler/dev_disk.cc
@@ -64,10 +64,11 @@ sanitize_label_string (WCHAR *s)
   /* Linux does not skip leading spaces. */
   return sanitize_string (s, L'\0', L' ', L'_', [] (WCHAR c) -> bool
     {
-      /* Labels may contain characters not allowed in filenames.
-	 Linux replaces spaces with \x20 which is not an option here. */
-      return !((0 <= c && c <= L' ') || c == L':' || c == L'/' || c == L'\\'
-	      || c == L'"');
+      /* Labels may contain characters not allowed in filenames.  Also
+         replace '#' to avoid that duplicate markers introduce new
+	 duplicates.  Linux replaces spaces with \x20 which is not an
+	 option here. */
+      return !(c == L'/' || c == L'\\' || c == L'#');
     }
   );
 }
@@ -304,8 +305,7 @@ partition_to_label_or_uuid(bool uuid, const UNICODE_STRING *drive_uname,
   const NTFS_VOLUME_DATA_BUFFER *nvdb =
     reinterpret_cast<const NTFS_VOLUME_DATA_BUFFER *>(ioctl_buf);
   if (uuid && DeviceIoControl (volhdl, FSCTL_GET_NTFS_VOLUME_DATA, nullptr, 0,
-			       ioctl_buf, NT_MAX_PATH, &bytes_read, nullptr)
-      && nvdb->VolumeSerialNumber.QuadPart)
+			       ioctl_buf, NT_MAX_PATH, &bytes_read, nullptr))
     {
       /* Print without any separator as on Linux. */
       __small_sprintf (name, "%016X", nvdb->VolumeSerialNumber.QuadPart);
@@ -327,13 +327,9 @@ partition_to_label_or_uuid(bool uuid, const UNICODE_STRING *drive_uname,
   FILE_FS_VOLUME_INFORMATION *ffvi =
     reinterpret_cast<FILE_FS_VOLUME_INFORMATION *>(ioctl_buf);
   if (uuid)
-    {
-      if (!ffvi->VolumeSerialNumber)
-	return false;
-      /* Print with separator as on Linux. */
-      __small_sprintf (name, "%04x-%04x", ffvi->VolumeSerialNumber >> 16,
-		       ffvi->VolumeSerialNumber & 0xffff);
-    }
+    /* Print with separator as on Linux. */
+    __small_sprintf (name, "%04x-%04x", ffvi->VolumeSerialNumber >> 16,
+		     ffvi->VolumeSerialNumber & 0xffff);
   else
     {
       /* Label is not null terminated. */
@@ -361,6 +357,20 @@ by_id_compare_name (const void *a, const void *b)
   return strcmp (ap->name, bp->name);
 }
 
+static int
+by_id_compare_name_drive_part (const void *a, const void *b)
+{
+  const by_id_entry *ap = reinterpret_cast<const by_id_entry *>(a);
+  const by_id_entry *bp = reinterpret_cast<const by_id_entry *>(b);
+  int cmp = strcmp (ap->name, bp->name);
+  if (cmp)
+    return cmp;
+  cmp = ap->drive - bp->drive;
+  if (cmp)
+    return cmp;
+  return ap->part - bp->part;
+}
+
 static by_id_entry *
 by_id_realloc (by_id_entry *p, size_t n)
 {
@@ -610,8 +620,9 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
   if (!table)
     return (errno_set ? -1 : 0);
 
-  /* Sort by name and remove duplicates. */
-  qsort (table, table_size, sizeof (*table), by_id_compare_name);
+  /* Sort by {name, drive, part} to ensure stable sort order. */
+  qsort (table, table_size, sizeof (*table), by_id_compare_name_drive_part);
+  /* Mark duplicate names. */
   for (unsigned i = 0; i < table_size; i++)
     {
       unsigned j = i + 1;
@@ -619,12 +630,13 @@ get_by_id_table (by_id_entry * &table, fhandler_dev_disk::dev_disk_location loc)
 	j++;
       if (j == i + 1)
 	continue;
-      /* Duplicate(s) found, remove all entries with this name. */
-      debug_printf ("removing duplicates %d-%d: '%s'", i, j - 1, table[i].name);
-      if (j < table_size)
-	memmove (table + i, table + j, (table_size - j) * sizeof (*table));
-      table_size -= j - i;
-      i--;
+      /* Duplicate(s) found, append "#N" to all entries.  This never
+	 introduces new duplicates because '#' never occurs in the
+	 original names. */
+      debug_printf ("mark duplicates %u-%u of '%s'", i, j - 1, table[i].name);
+      size_t len = strlen (table[i].name);
+      for (unsigned k = i; k < j; k++)
+	__small_sprintf (table[k].name + len, "#%u", k - i);
     }
 
   debug_printf ("table_size: %d", table_size);
-- 
2.42.1

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Corinna Vinschen]

On Nov 21 19:31, Christian Franke wrote:
> Corinna Vinschen wrote:
> > Hi Christian,
> > 
> > Looks good, but I just realized that I was already wondering about the
> > sanitization and forgot to talk about it:
> > 
> > On Nov 21 12:24, Christian Franke wrote:
> > > diff --git a/winsup/cygwin/fhandler/dev_disk.cc b/winsup/cygwin/fhandler/dev_disk.cc
> > > index c5d72816f..d12ac52fa 100644
> > > --- a/winsup/cygwin/fhandler/dev_disk.cc
> > > +++ b/winsup/cygwin/fhandler/dev_disk.cc
> > > @@ -64,10 +64,12 @@ sanitize_label_string (WCHAR *s)
> > >     /* Linux does not skip leading spaces. */
> > >     return sanitize_string (s, L'\0', L' ', L'_', [] (WCHAR c) -> bool
> > >       {
> > > -      /* Labels may contain characters not allowed in filenames.
> > > -	 Linux replaces spaces with \x20 which is not an option here. */
> > > +      /* Labels may contain characters not allowed in filenames.  Also
> > Apart from slash and backslash, we don't have this problem in Cygwin,
> > usually.  Even control characters are no problem.  All chars not allowed
> > in filenames are just transposed into the Unicode private use area, as
> > per strfuncs.cc, line 20ff on the way to storage, and back when reading
> > the names from storage.  This, and especially in a virtual filesystem
> > like /proc, there's no reason to avoid these characters.
> 
> Thanks for clarification.
> 
> 
> > 
> > > +         replace '#' to avoid that duplicate markers introduce new
> > > +	 duplicates.  Linux replaces spaces with \x20 which is not an
> > > +	 option here. */
> > >         return !((0 <= c && c <= L' ') || c == L':' || c == L'/' || c == L'\\'
> > > -	      || c == L'"');
> > > +	      || c == L'#' || c == L'"');
> > If you really want to avoid chars not allowed in DOS filenames, the
> > list seems incomplete, missing '<', '>', '?', '*', '|'.
> > 
> > But as I said, there's really no reason for that.  I simply reduced the
> > above expression to
> > 
> >    return !(c == L'/' || c == L'\\' || c == L'#');
> > 
> > and created a disk label
> > 
> >    test"foo*bar?baz:"
> > 
> > It works nicely, including stuff like
> > 
> >    $ ls *\"*
> >    $ ls *\**
> > 
> > So, I can push it as is, or we just allow everything and the kitchen sink
> > as per the reduced filter expression.  What do you prefer?
> 
> The latter - patch attached.

Pushed.

Thanks a lot,
Corinna

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Corinna Vinschen]

Hi Christian,


On second thought...

I had a bad night tonight and was thinking a long time about this and
that.  It suddenly occured to me that there might be another problem
with this approach, attaching ordinals to the label name.

Assuming you have a single filesystem labled "VOLUME" which is on a
fixed disk.  So you get something like this:

  $ ls -l /dev/disk/by-label
  total 0
  lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:09  VOLUME -> ../../sdb1
  lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:10  root -> ../../sda3

Now you insert an USB Stick with a FAT32 filesystem, also labeled
"VOLUME".  Now you get something like this:

  $ ls -l /dev/disk/by-label
  total 0
  lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:12 'VOLUME#0' -> ../../sdb1
  lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:12 'VOLUME#1' -> ../../sdc1
  lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:10  root -> ../../sda3

So the label name changes, depending on inserting or removing another
partition.

Not saying I have a good solution myself, so I wonder if we should just
let it slip, but I thought we should at least talk about it...


Thanks,
Corinna

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Christian Franke]

Hi Corinna,

Corinna Vinschen wrote:
> Hi Christian,
>
>
> On second thought...
>
> I had a bad night tonight and was thinking a long time about this and
> that.  It suddenly occured to me that there might be another problem
> with this approach, attaching ordinals to the label name.
>
> Assuming you have a single filesystem labled "VOLUME" which is on a
> fixed disk.  So you get something like this:
>
>    $ ls -l /dev/disk/by-label
>    total 0
>    lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:09  VOLUME -> ../../sdb1
>    lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:10  root -> ../../sda3
>
> Now you insert an USB Stick with a FAT32 filesystem, also labeled
> "VOLUME".  Now you get something like this:
>
>    $ ls -l /dev/disk/by-label
>    total 0
>    lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:12 'VOLUME#0' -> ../../sdb1
>    lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:12 'VOLUME#1' -> ../../sdc1
>    lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:10  root -> ../../sda3
>
> So the label name changes, depending on inserting or removing another
> partition.

This is intentional. If the first duplicate appears, it is IMO better to 
also replace the original name to show that a duplicate exists.


>
> Not saying I have a good solution myself, so I wonder if we should just
> let it slip, but I thought we should at least talk about it...

Users should be aware that unspecific label names like VOLUME could not 
be used as a persistent link if drives are changed.

Same may apply to by-partuuid names as preformatted SD-cards and USB 
flash drives may have a null MBR serial number.

Regards,
Christian

Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks

[Corinna Vinschen]

On Nov 22 17:31, Christian Franke wrote:
> Hi Corinna,
> 
> Corinna Vinschen wrote:
> > Hi Christian,
> > 
> > 
> > On second thought...
> > 
> > I had a bad night tonight and was thinking a long time about this and
> > that.  It suddenly occured to me that there might be another problem
> > with this approach, attaching ordinals to the label name.
> > 
> > Assuming you have a single filesystem labled "VOLUME" which is on a
> > fixed disk.  So you get something like this:
> > 
> >    $ ls -l /dev/disk/by-label
> >    total 0
> >    lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:09  VOLUME -> ../../sdb1
> >    lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:10  root -> ../../sda3
> > 
> > Now you insert an USB Stick with a FAT32 filesystem, also labeled
> > "VOLUME".  Now you get something like this:
> > 
> >    $ ls -l /dev/disk/by-label
> >    total 0
> >    lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:12 'VOLUME#0' -> ../../sdb1
> >    lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:12 'VOLUME#1' -> ../../sdc1
> >    lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:10  root -> ../../sda3
> > 
> > So the label name changes, depending on inserting or removing another
> > partition.
> 
> This is intentional. If the first duplicate appears, it is IMO better to
> also replace the original name to show that a duplicate exists.
> 
> 
> > 
> > Not saying I have a good solution myself, so I wonder if we should just
> > let it slip, but I thought we should at least talk about it...
> 
> Users should be aware that unspecific label names like VOLUME could not be
> used as a persistent link if drives are changed.
> 
> Same may apply to by-partuuid names as preformatted SD-cards and USB flash
> drives may have a null MBR serial number.

Makes total sense.


Thanks,
Corinna