[PATCH] Cygwin: Add /dev/disk/by-id symlinks

[Christian Franke]

Corinna Vinschen wrote:
> On Nov  3 10:55, Corinna Vinschen wrote:
>> Hi Christian,
>>
>> On Oct  3 14:39, Christian Franke wrote:
>>> Christian Franke wrote:
>>>> This is a first attempt to partly emulate the Linux directory
>>>> /dev/disk/by-id. Useful to make sure the correct device is accessed in
>>>> conjunction with dd, ddrescue, fdisk, ....
>>> Attached is the second attempt.
>>>
>>>
>>>> The additional '*-partN' links to partitions are not yet included.
>>> These are now included.
>>>
>>>
>>>> This only works properly if Win32 path '\\.\PhysicalDriveN' is always
>>>> trivially mapped to NT path '\Device\HarddiskN\Partition0'.
>>>> IOCTL_STORAGE_QUERY_PROPERTY with a handle from NtOpenFile(.,
>>>> READ_CONTROL,...) instead of CreateFile(., 0, ...) did not work with all
>>>> drivers. With stornvme.sys, it fails with permission denied. Perhaps
>>>> other permission bits are required for NtOpenFile(). Thanks for any info
>>>> regarding this.
>>> According to NtQueryObject(., ObjectBasicInformation, ...), using
>>> NtOpenFile(., MAXIMUM_ALLOWED, ...) without admin rights sets GrantedAccess
>>> to 0x001200a0 (FILE_EXECUTE|FILE_READ_ATTRIBUTES|READ_CONTROL|SYNCHRONIZE).
>>> For some unknown reason, NVMe drives behind stornvme.sys additionally
>>> require SYNCHRONIZE to use IOCTL_STORAGE_QUERY_PROPERTY. Possibly a harmless
>>> bug in the access check somewhere in the NVMe stack.
>>>
>>> The disk scanning from the first patch has been reworked based on code
>>> borrowed from proc.cc:format_proc_partitions(). For the longer term, it may
>>> make sense to provide one flexible scanning function for /dev/sdXN,
>>> /proc/partitions and /proc/disk/by-id.
>> I applied your patch locally (patch looks pretty well, btw) but found

Thanks!
Meantime I found 4 missing NtClose() in the main loop of 
get_by_id_table(). Will be fixed in the next version of the patch.


>> that /dev/disk/by-id is empty, even when running with admin rights.

Admin rights should not be necessary for IOCTL_STORAGE_QUERY_PROPERTY.


>>
>> I ran this on Windows 11 and Windows 2K19 in a QEMU/KVM VM.  A
>> \Device\Harddisk0\Partition0 symlink pointing to \Device\Harddisk0\DR0
>> exists in both cases.  I straced it, and found the following debug
>> output:
>>
>>    1015 1155432 [main] ls 361 stordesc_to_id_name: Harddisk0\Partition0: 'Red_Hat' 'VirtIO' '' (ignored)
>>
>> Is that really desired?

Yes - if IOCTL_STORAGE_QUERY_PROPERTY{... PropertyStandardQuery} does 
not return a serial number (''), the device is intentionally ignored.


> Thread 1 "ls" hit Breakpoint 2, stordesc_to_id_name (upath=0x7ffffc500,
>      ioctl_buf=0x10e0720 "(", name=...)
>      at /home/corinna/src/cygwin/vanilla/winsup/cygwin/fhandler/dev_disk.cc:44
> 44        const STORAGE_DEVICE_DESCRIPTOR *desc =
> (gdb) n
> 47        int vendor_len = 0, product_len = 0, serial_len = 0;
> (gdb)
> 48        if (desc->VendorIdOffset)
> (gdb)
> 49          vendor_len = sanitize_id_string (ioctl_buf + desc->VendorIdOffset);
> (gdb)
> 50        if (desc->ProductIdOffset)
> (gdb)
> 51          product_len = sanitize_id_string (ioctl_buf + desc->ProductIdOffset);
> (gdb)
> 52        if (desc->SerialNumberOffset)
> (gdb)
> 53          serial_len = sanitize_id_string (ioctl_buf + desc->SerialNumberOffset);

If possibly, please check whether (desc->SerialNumberOffset) is 0 or 
(ioctl_buf + desc->SerialNumberOffset) points to '\0' or a string of 
spaces. If no, there is possibly something wrong in sanitize_id_string().


> (gdb)
> 55        bool valid = (4 <= vendor_len + product_len && 4 <= serial_len
> (gdb) p vendor_len
> $1 = 7
> (gdb) p product_len
> $2 = 6
> (gdb) p serial_len
> $3 = 0
> (gdb) n
> [New Thread 3944.0x1958]
> 56                      && (20 + vendor_len + 1 + product_len + 1 + serial_len + 10)
> (gdb) n
> 55        bool valid = (4 <= vendor_len + product_len && 4 <= serial_len
> (gdb)
> 55        bool valid = (4 <= vendor_len + product_len && 4 <= serial_len
> (gdb)
> 58        debug_printf ("%S: '%s' '%s' '%s'%s", upath,
> (gdb) p valid
> $4 = false

-- 
Regards,
Christian