ata path_id

[yodayox]

dropped on the way log time ago and nobody seems to care about..
https://forums.gentoo.org/viewtopic-t-1053170.html
so restored by.. me, as follow:

--- a/src/udev/udev-builtin-path_id.c	2022-06-14 02:44:42.000000000 +0300
+++ b/src/udev/udev-builtin-path_id.c	2022-11-27 11:58:38.190434094 +0200
@@ -390,6 +390,55 @@
         return hostdev;
 }
 
+static struct udev_device *handle_scsi_ata(struct udev_device *parent, char **path, char **compat_path) {
+        struct udev_device *targetdev, *target_parent;
+        struct udev_device *atadev = NULL;
+        struct udev *udev = udev_device_get_udev(parent); 
+        const char *port_no, *sysname, *name;
+        unsigned host, bus, target, lun;
+
+        assert(parent);
+        assert(path);
+        
+        
+        name = udev_device_get_sysname(parent);
+        if (sscanf(name, "%u:%u:%u:%u", &host, &bus, &target, &lun) != 4)
+                return NULL;
+        
+        targetdev = udev_device_get_parent_with_subsystem_devtype(parent, "scsi", "scsi_host");
+                if (targetdev == NULL)
+                return NULL;
+
+        target_parent = udev_device_get_parent(targetdev);
+                if (target_parent == NULL)
+                return NULL;
+
+        sysname = udev_device_get_sysname(target_parent);
+                if (sysname == NULL)
+                return NULL;
+        
+        atadev = udev_device_new_from_subsystem_sysname(udev, "ata_port", udev_device_get_sysname(target_parent));
+                if (atadev == NULL)
+                return NULL;
+
+        port_no = udev_device_get_sysattr_value(atadev, "port_no");
+                if (port_no == NULL)
+                return NULL;
+
+        if (bus != 0)
+                /* Devices behind port multiplier have a bus != 0 */
+                path_prepend(path, "ata-%s.%u.0", port_no, bus);
+        else
+                /* Master/slave are distinguished by target id */
+                path_prepend(path, "ata-%s.%u", port_no, target);
+
+        /* old compatible persistent link for ATA devices */
+        if (compat_path)
+                path_prepend(compat_path, "ata-%s", port_no);
+
+        return parent;
+}
+
 static struct udev_device *handle_scsi_hyperv(struct udev_device *parent, char **path) {
         struct udev_device *hostdev;
         struct udev_device *vmbusdev;
@@ -426,7 +475,7 @@
         return parent;
 }
 
-static struct udev_device *handle_scsi(struct udev_device *parent, char **path, bool *supported_parent) {
+static struct udev_device *handle_scsi(struct udev_device *parent, char **path, char **compat_path, bool *supported_parent) {
         const char *devtype;
         const char *name;
         const char *id;
@@ -465,19 +514,8 @@
                 goto out;
         }
 
-        /*
-         * We do not support the ATA transport class, it uses global counters
-         * to name the ata devices which numbers spread across multiple
-         * controllers.
-         *
-         * The real link numbers are not exported. Also, possible chains of ports
-         * behind port multipliers cannot be composed that way.
-         *
-         * Until all that is solved at the kernel level, there are no by-path/
-         * links for ATA devices.
-         */
         if (strstr(name, "/ata") != NULL) {
-                parent = NULL;
+                parent = handle_scsi_ata(parent, path, compat_path);
                 goto out;
         }
 
@@ -581,6 +619,7 @@
         char *path = NULL;
         bool supported_transport = false;
         bool supported_parent = false;
+        _cleanup_free_ char *compat_path = NULL;
 
         /* S390 ccw bus */
         parent = udev_device_get_parent_with_subsystem_devtype(dev, "ccw", NULL);
@@ -600,7 +639,7 @@
                 } else if (streq(subsys, "scsi_tape")) {
                         handle_scsi_tape(parent, &path);
                 } else if (streq(subsys, "scsi")) {
-                        parent = handle_scsi(parent, &path, &supported_parent);
+                        parent = handle_scsi(parent, &path, &compat_path, &supported_parent);
                         supported_transport = true;
                 } else if (streq(subsys, "cciss")) {
                         parent = handle_cciss(parent, &path);
@@ -616,23 +655,33 @@
                         parent = skip_subsystem(parent, "serio");
                 } else if (streq(subsys, "pci")) {
                         path_prepend(&path, "pci-%s", udev_device_get_sysname(parent));
+                        if (compat_path)
+                                path_prepend(&compat_path, "pci-%s", udev_device_get_sysname(parent));
                         parent = skip_subsystem(parent, "pci");
                         supported_parent = true;
                 } else if (streq(subsys, "platform")) {
                         path_prepend(&path, "platform-%s", udev_device_get_sysname(parent));
+                        if (compat_path)
+                                path_prepend(&compat_path, "platform-%s", udev_device_get_sysname(parent));
                         parent = skip_subsystem(parent, "platform");
                         supported_transport = true;
                         supported_parent = true;
                 } else if (streq(subsys, "acpi")) {
                         path_prepend(&path, "acpi-%s", udev_device_get_sysname(parent));
+                        if (compat_path)
+                                path_prepend(&compat_path, "acpi-%s", udev_device_get_sysname(parent));
                         parent = skip_subsystem(parent, "acpi");
                         supported_parent = true;
                 } else if (streq(subsys, "xen")) {
                         path_prepend(&path, "xen-%s", udev_device_get_sysname(parent));
+                        if (compat_path)
+                                path_prepend(&compat_path, "xen-%s", udev_device_get_sysname(parent));
                         parent = skip_subsystem(parent, "xen");
                         supported_parent = true;
                 } else if (streq(subsys, "scm")) {
                         path_prepend(&path, "scm-%s", udev_device_get_sysname(parent));
+                        if (compat_path)
+                                path_prepend(&compat_path, "scm-%s", udev_device_get_sysname(parent));
                         parent = skip_subsystem(parent, "scm");
                         supported_transport = true;
                         supported_parent = true;
@@ -694,10 +743,16 @@
 
                 udev_builtin_add_property(dev, test, "ID_PATH", path);
                 udev_builtin_add_property(dev, test, "ID_PATH_TAG", tag);
-                free(path);
-                return EXIT_SUCCESS;
         }
-        return EXIT_FAILURE;
+        /*
+         * Compatible link generation for ATA devices
+         * we assign compat_link to the env variable
+         * ID_PATH_ATA_COMPAT
+         */
+        if (compat_path)
+                udev_builtin_add_property(dev, test, "ID_PATH_ATA_COMPAT", compat_path);
+
+        return 0;
 }
 
 const struct udev_builtin udev_builtin_path_id = {

in order to be shipped in next version of eudev

[bbonev]

Thanks for reporting this.

At a first glance, path will leak. Also changing return EXIT_SUCCESS to return 0 breaks the style of the source.

Also it seems that there are more changes in systemd/src/udev/udev-builtin-path_id.c not addressed in this patch (I assume that this patch is a feature merge).

Please convert to a Pull Request and close this issue - PRs are much easier to review and change until they get ready to be merged.

[yodayox]

yes the are more changes but this is what was dropped in the past
restored from 252 and.. it works fine to me .

[yodayox]

you said path will leak
then the last part of the patch above should look like the follow one (also restored the "style")

@@ -694,6 +743,16 @@
 
                 udev_builtin_add_property(dev, test, "ID_PATH", path);
                 udev_builtin_add_property(dev, test, "ID_PATH_TAG", tag);
+        /*
+         * Compatible link generation for ATA devices
+         * we assign compat_link to the env variable
+         * ID_PATH_ATA_COMPAT
+         */
+        if (compat_path) {
+                udev_builtin_add_property(dev, test, "ID_PATH_ATA_COMPAT", compat_path);
+                free(compat_path);
+        }
+                                                                        
                 free(path);
                 return EXIT_SUCCESS;
         }

i have no ideea how to create a pull request and.. don't need to know :-)

[bbonev]

Better make it consistent with compat_path:

_cleanup_free_ char *path = NULL;

and do not use free at all, because _cleanup_free_ will automagically free it after it comes out of scope.

About the pull request - that is part of the current project workflow, and if you do not know how to do that, it is perfectly fine, someone of us will do it when they have time. The patch above needs some more work to sync the other changes and then it will be good to merge.

[yodayox]

yes you're right using free() is a pain in the a..s for udevd .. it seems that it cannot use that variable after free
so used cleanup_free for both var (path and compat_path)
now it looks and acts better..

--- a/src/udev/udev-builtin-path_id.c	2022-11-13 03:59:04.000000000 +0200
+++ b/src/udev/udev-builtin-path_id.c	2022-11-29 22:08:15.593308302 +0200
@@ -390,6 +390,55 @@
         return hostdev;
 }
 
+static struct udev_device *handle_scsi_ata(struct udev_device *parent, char **path, char **compat_path) {
+        struct udev_device *targetdev, *target_parent;
+        struct udev_device *atadev = NULL;
+        struct udev *udev = udev_device_get_udev(parent); 
+        const char *port_no, *sysname, *name;
+        unsigned host, bus, target, lun;
+
+        assert(parent);
+        assert(path);
+        
+        
+        name = udev_device_get_sysname(parent);
+        if (sscanf(name, "%u:%u:%u:%u", &host, &bus, &target, &lun) != 4)
+                return NULL;
+        
+        targetdev = udev_device_get_parent_with_subsystem_devtype(parent, "scsi", "scsi_host");
+                if (targetdev == NULL)
+                return NULL;
+
+        target_parent = udev_device_get_parent(targetdev);
+                if (target_parent == NULL)
+                return NULL;
+
+        sysname = udev_device_get_sysname(target_parent);
+                if (sysname == NULL)
+                return NULL;
+        
+        atadev = udev_device_new_from_subsystem_sysname(udev, "ata_port", udev_device_get_sysname(target_parent));
+                if (atadev == NULL)
+                return NULL;
+
+        port_no = udev_device_get_sysattr_value(atadev, "port_no");
+                if (port_no == NULL)
+                return NULL;
+
+        if (bus != 0)
+                /* Devices behind port multiplier have a bus != 0 */
+                path_prepend(path, "ata-%s.%u.0", port_no, bus);
+        else
+                /* Master/slave are distinguished by target id */
+                path_prepend(path, "ata-%s.%u", port_no, target);
+
+        /* old compatible persistent link for ATA devices */
+        if (compat_path)
+                path_prepend(compat_path, "ata-%s", port_no);
+
+        return parent;
+}
+
 static struct udev_device *handle_scsi_hyperv(struct udev_device *parent, char **path) {
         struct udev_device *hostdev;
         struct udev_device *vmbusdev;
@@ -426,7 +475,7 @@
         return parent;
 }
 
-static struct udev_device *handle_scsi(struct udev_device *parent, char **path, bool *supported_parent) {
+static struct udev_device *handle_scsi(struct udev_device *parent, char **path, char **compat_path, bool *supported_parent) {
         const char *devtype;
         const char *name;
         const char *id;
@@ -465,19 +514,8 @@
                 goto out;
         }
 
-        /*
-         * We do not support the ATA transport class, it uses global counters
-         * to name the ata devices which numbers spread across multiple
-         * controllers.
-         *
-         * The real link numbers are not exported. Also, possible chains of ports
-         * behind port multipliers cannot be composed that way.
-         *
-         * Until all that is solved at the kernel level, there are no by-path/
-         * links for ATA devices.
-         */
         if (strstr(name, "/ata") != NULL) {
-                parent = NULL;
+                parent = handle_scsi_ata(parent, path, compat_path);
                 goto out;
         }
 
@@ -578,10 +616,11 @@
 
 static int builtin_path_id(struct udev_device *dev, int argc __attribute__((unused)), char *argv[] __attribute__((unused)), bool test) {
         struct udev_device *parent;
-        char *path = NULL;
+        _cleanup_free_ char *path = NULL;
         bool supported_transport = false;
         bool supported_parent = false;
-
+        _cleanup_free_ char *compat_path = NULL;
+        
         /* S390 ccw bus */
         parent = udev_device_get_parent_with_subsystem_devtype(dev, "ccw", NULL);
         if (parent != NULL) {
@@ -600,7 +639,7 @@
                 } else if (streq(subsys, "scsi_tape")) {
                         handle_scsi_tape(parent, &path);
                 } else if (streq(subsys, "scsi")) {
-                        parent = handle_scsi(parent, &path, &supported_parent);
+                        parent = handle_scsi(parent, &path, &compat_path, &supported_parent);
                         supported_transport = true;
                 } else if (streq(subsys, "cciss")) {
                         parent = handle_cciss(parent, &path);
@@ -616,23 +655,33 @@
                         parent = skip_subsystem(parent, "serio");
                 } else if (streq(subsys, "pci")) {
                         path_prepend(&path, "pci-%s", udev_device_get_sysname(parent));
+                        if (compat_path)
+                                path_prepend(&compat_path, "pci-%s", udev_device_get_sysname(parent));
                         parent = skip_subsystem(parent, "pci");
                         supported_parent = true;
                 } else if (streq(subsys, "platform")) {
                         path_prepend(&path, "platform-%s", udev_device_get_sysname(parent));
+                        if (compat_path)
+                                path_prepend(&compat_path, "platform-%s", udev_device_get_sysname(parent));
                         parent = skip_subsystem(parent, "platform");
                         supported_transport = true;
                         supported_parent = true;
                 } else if (streq(subsys, "acpi")) {
                         path_prepend(&path, "acpi-%s", udev_device_get_sysname(parent));
+                        if (compat_path)
+                                path_prepend(&compat_path, "acpi-%s", udev_device_get_sysname(parent));
                         parent = skip_subsystem(parent, "acpi");
                         supported_parent = true;
                 } else if (streq(subsys, "xen")) {
                         path_prepend(&path, "xen-%s", udev_device_get_sysname(parent));
+                        if (compat_path)
+                                path_prepend(&compat_path, "xen-%s", udev_device_get_sysname(parent));
                         parent = skip_subsystem(parent, "xen");
                         supported_parent = true;
                 } else if (streq(subsys, "scm")) {
                         path_prepend(&path, "scm-%s", udev_device_get_sysname(parent));
+                        if (compat_path)
+                                path_prepend(&compat_path, "scm-%s", udev_device_get_sysname(parent));
                         parent = skip_subsystem(parent, "scm");
                         supported_transport = true;
                         supported_parent = true;
@@ -694,10 +743,18 @@
 
                 udev_builtin_add_property(dev, test, "ID_PATH", path);
                 udev_builtin_add_property(dev, test, "ID_PATH_TAG", tag);
-                free(path);
-                return EXIT_SUCCESS;
+        /*
+         * Compatible link generation for ATA devices
+         * we assign compat_link to the env variable
+         * ID_PATH_ATA_COMPAT
+         */
+        if (compat_path) {
+                udev_builtin_add_property(dev, test, "ID_PATH_ATA_COMPAT", compat_path);
+        }
+        return EXIT_SUCCESS;
         }
         return EXIT_FAILURE;
+
 }
 
 const struct udev_builtin udev_builtin_path_id = {

[mrled]

I hit this on an Alpine Linux system with eudev. SATA disks are missing, and I also notice that NVME disks do not show up in /dev/disk/by-path.

Running eudev-3.2.11-r7 against kernel 5.15.103-0-lts, both from Alpine packages. I have two USB disks (sdb and sdc), 1 SATA disk (sda), and 1 NVME disk (nvme0n1), but only the USB disks show up in /dev/disk/by-path.

kenasus:~# ls -alF /dev/disk/by-path
total 0
drwxr-xr-x 2 root root 120 Mar 20 21:40 ./
drwxrwxrwx 8 root root 160 Mar 20 21:40 ../
lrwxrwxrwx 1 root root   9 Mar 21 00:41 pci-0000:00:14.0-usb-0:3:1.0-scsi-0:0:0:0 -> ../../sdb
lrwxrwxrwx 1 root root   9 Mar 21 00:41 pci-0000:00:14.0-usb-0:7:1.0-scsi-0:0:0:0 -> ../../sdc
lrwxrwxrwx 1 root root  10 Mar 21 00:41 pci-0000:00:14.0-usb-0:7:1.0-scsi-0:0:0:0-part1 -> ../../sdc1
lrwxrwxrwx 1 root root  10 Mar 21 00:41 pci-0000:00:14.0-usb-0:7:1.0-scsi-0:0:0:0-part2 -> ../../sdc2

kenasus:~# lsblk
NAME                                 MAJ:MIN RM   SIZE RO TYPE  MOUNTPOINTS
loop0                                  7:0    0 106.2M  1 loop  /.modloop
sda                                    8:0    0 238.5G  0 disk  
sdb                                    8:16   1   3.7G  0 disk  /mnt/psyops-secret/mount
sdc                                    8:32   1   7.5G  0 disk  
├─sdc1                                 8:33   1   696M  0 part  /media/sdc1
└─sdc2                                 8:34   1   1.4M  0 part  
nvme0n1                              259:0    0 931.5G  0 disk  
├─nvme0n1p1                          259:1    0   256G  0 part  
│ └─nvme0n1p1_crypt                  253:0    0   256G  0 crypt 
│   └─psyopsos_datadiskvg-datadisklv 253:1    0   256G  0 lvm   /etc/rancher
│                                                               /var/lib/containerd
│                                                               /var/lib/rancher/k3s
│                                                               /psyopsos-data
└─nvme0n1p2                          259:2    0 675.5G  0 part  

[yodayox]

i'm on PCLinuxOS and tested on kernel 5.4.236 and 6.1.20 - no issues here
sdb > usb
sda > hdd
sr > cd-rom

[yoda@localhost ~]$ ls -alF /dev/disk/by-path
total 0
drwxr-xr-x 2 root root 400 Mar 22 08:58 ./
drwxr-xr-x 7 root root 140 Mar 18 18:38 ../
lrwxrwxrwx 1 root root   9 Mar 22 08:58 pci-0000:00:14.0-usb-0:10:1.0-scsi-0:0:0:0 -> ../../sdb
lrwxrwxrwx 1 root root  10 Mar 22 08:58 pci-0000:00:14.0-usb-0:10:1.0-scsi-0:0:0:0-part1 -> ../../sdb1
lrwxrwxrwx 1 root root   9 Mar 18 18:38 pci-0000:00:17.0-ata-1 -> ../../sda
lrwxrwxrwx 1 root root   9 Mar 18 18:38 pci-0000:00:17.0-ata-1.0 -> ../../sda
lrwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1.0-part1 -> ../../sda1
lrwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1.0-part2 -> ../../sda2
lrwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1.0-part5 -> ../../sda5
rwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1.0-part6 -> ../../sda6
lrwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1.0-part7 -> ../../sda7
lrwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1.0-part8 -> ../../sda8
lrwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1-part1 -> ../../sda1
lrwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1-part2 -> ../../sda2
lrwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1-part5 -> ../../sda5
lrwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1-part6 -> ../../sda6
lrwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1-part7 -> ../../sda7
lrwxrwxrwx 1 root root  10 Mar 18 18:38 pci-0000:00:17.0-ata-1-part8 -> ../../sda8
lrwxrwxrwx 1 root root   9 Mar 18 18:38 pci-0000:00:17.0-ata-2 -> ../../sr0
lrwxrwxrwx 1 root root   9 Mar 18 18:38 pci-0000:00:17.0-ata-2.0 -> ../../sr0

[EDIT]
oooppss.. the above patches are NOT in upstream so that's why you can't see those by-path links
on the other hand PCLinuxOS is using the above patches... you can take a look at
http://ftp.fau.de/pclinuxos/pclinuxos/srpms/SRPMS.pclos/udev-243-11pclos2023.src.rpm

[mscdex]

I just wanted to voice my support for these changes as I was running into an issue with virtio-scsi qemu dvdrom devices not showing up in /dev/disk/by-path, but with the changes detailed in #242 (comment) the symlink is now visible.

@yodayox Are you still not interested in submitting a PR? If so, I can submit one (although I am not familiar with the code being changed here) as I'd rather not have to maintain a patch on top of eudev...

[bbonev]

Please see my comment from Nov 2022 - most of the problems were solved but there is one thing left:

Also it seems that there are more changes in systemd/src/udev/udev-builtin-path_id.c not addressed in this patch (I assume that this patch is a feature merge).

My preference is not to merge something half baked. I'd prefer a PR but will also accept a patch as long as it addresses the above. In the case of a patch, I will make the PR and it will need a review and approval.

About the PCLinuxOS SRPM - that contains different patches from the one proposed above. Maybe they are worth merging or maybe not. If you are interested, propose them as PRs...

[yodayox]

the nvme patch was corrected as #273 ( just a wrong if statement..) - PCLinuxOS already have it
as for ATA, #275 looks OK to me...