[PATCH v2 0/4] Allow guest-get-fsinfo also for non-PCI devices

[PATCH v2 0/4] Allow guest-get-fsinfo also for non-PCI devices

[Thomas Huth]

The information that can be retrieved via UDEV is also usable for non-PCI
devices. So let's allow build_guest_fsinfo_for_real_device() on non-PCI
devices, too. This is required to fix the bug that CCW devices show up
without "Target" when running libvirt's "virsh domfsinfo" command (see
https://bugzilla.redhat.com/show_bug.cgi?id=1755075 for details).

v2:
 - Use g_new0 instead of g_malloc0 (as suggested by Daniel)
 - Init fields to -1 explicitely, not via memset (Daniel)
 - Add the fourth patch to also fill in virtio information on s390x

Thomas Huth (4):
  qga/qapi-schema: Document -1 for invalid PCI address fields
  qga/commands-posix: Rework build_guest_fsinfo_for_real_device()
    function
  qga/commands-posix: Move the udev code from the pci to the generic
    function
  qga/commands-posix: Support fsinfo for non-PCI virtio devices, too

 qga/commands-posix.c | 157 ++++++++++++++++++++++++++++++-------------
 qga/qapi-schema.json |   2 +-
 2 files changed, 110 insertions(+), 49 deletions(-)

-- 
2.18.1

[PATCH v2 1/4] qga/qapi-schema: Document -1 for invalid PCI address fields

[Thomas Huth]

The "guest-get-fsinfo" could also be used for non-PCI devices in the
future. And the code in GuestPCIAddress() in qga/commands-win32.c seems
to be using "-1" for fields that it can not determine already. Thus
let's properly document "-1" as value for invalid PCI address fields.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qga/qapi-schema.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 4be9aad48e..408a662ea5 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -846,7 +846,7 @@
 ##
 # @GuestDiskAddress:
 #
-# @pci-controller: controller's PCI address
+# @pci-controller: controller's PCI address (fields are set to -1 if invalid)
 # @bus-type: bus type
 # @bus: bus id
 # @target: target id
-- 
2.18.1

[PATCH v2 2/4] qga/commands-posix: Rework build_guest_fsinfo_for_real_device() function

[Thomas Huth]

We are going to support non-PCI devices soon. For this we need to split
the generic GuestDiskAddress and GuestDiskAddressList memory allocation
and list chaining into a separate function first.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qga/commands-posix.c | 65 ++++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 24 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1a62a3a70d..1a42ec8171 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -861,28 +861,30 @@ static int build_hosts(char const *syspath, char const *host, bool ata,
     return i;
 }
 
-/* Store disk device info specified by @sysfs into @fs */
-static void build_guest_fsinfo_for_real_device(char const *syspath,
-                                               GuestFilesystemInfo *fs,
-                                               Error **errp)
+/*
+ * Store disk device info for devices on the PCI bus.
+ * Returns true if information has been stored, or false for failure.
+ */
+static bool build_guest_fsinfo_for_pci_dev(char const *syspath,
+                                           GuestDiskAddress *disk,
+                                           Error **errp)
 {
     unsigned int pci[4], host, hosts[8], tgt[3];
     int i, nhosts = 0, pcilen;
-    GuestDiskAddress *disk;
-    GuestPCIAddress *pciaddr;
-    GuestDiskAddressList *list = NULL;
+    GuestPCIAddress *pciaddr = disk->pci_controller;
     bool has_ata = false, has_host = false, has_tgt = false;
     char *p, *q, *driver = NULL;
 #ifdef CONFIG_LIBUDEV
     struct udev *udev = NULL;
     struct udev_device *udevice = NULL;
 #endif
+    bool ret = false;
 
     p = strstr(syspath, "/devices/pci");
     if (!p || sscanf(p + 12, "%*x:%*x/%x:%x:%x.%x%n",
                      pci, pci + 1, pci + 2, pci + 3, &pcilen) < 4) {
         g_debug("only pci device is supported: sysfs path '%s'", syspath);
-        return;
+        return false;
     }
 
     p += 12 + pcilen;
@@ -903,7 +905,7 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
         }
 
         g_debug("unsupported driver or sysfs path '%s'", syspath);
-        return;
+        return false;
     }
 
     p = strstr(syspath, "/target");
@@ -929,18 +931,11 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
         }
     }
 
-    pciaddr = g_malloc0(sizeof(*pciaddr));
     pciaddr->domain = pci[0];
     pciaddr->bus = pci[1];
     pciaddr->slot = pci[2];
     pciaddr->function = pci[3];
 
-    disk = g_malloc0(sizeof(*disk));
-    disk->pci_controller = pciaddr;
-
-    list = g_malloc0(sizeof(*list));
-    list->value = disk;
-
 #ifdef CONFIG_LIBUDEV
     udev = udev_new();
     udevice = udev_device_new_from_syspath(udev, syspath);
@@ -1018,21 +1013,43 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
         goto cleanup;
     }
 
-    list->next = fs->disk;
-    fs->disk = list;
-    goto out;
+    ret = true;
 
 cleanup:
-    if (list) {
-        qapi_free_GuestDiskAddressList(list);
-    }
-out:
     g_free(driver);
 #ifdef CONFIG_LIBUDEV
     udev_unref(udev);
     udev_device_unref(udevice);
 #endif
-    return;
+    return ret;
+}
+
+/* Store disk device info specified by @sysfs into @fs */
+static void build_guest_fsinfo_for_real_device(char const *syspath,
+                                               GuestFilesystemInfo *fs,
+                                               Error **errp)
+{
+    GuestDiskAddress *disk;
+    GuestPCIAddress *pciaddr;
+    GuestDiskAddressList *list = NULL;
+    bool has_hwinf;
+
+    pciaddr = g_new0(GuestPCIAddress, 1);
+
+    disk = g_new0(GuestDiskAddress, 1);
+    disk->pci_controller = pciaddr;
+
+    list = g_new0(GuestDiskAddressList, 1);
+    list->value = disk;
+
+    has_hwinf = build_guest_fsinfo_for_pci_dev(syspath, disk, errp);
+
+    if (has_hwinf) {
+        list->next = fs->disk;
+        fs->disk = list;
+    } else {
+        qapi_free_GuestDiskAddressList(list);
+    }
 }
 
 static void build_guest_fsinfo_for_device(char const *devpath,
-- 
2.18.1

[PATCH v2 3/4] qga/commands-posix: Move the udev code from the pci to the generic function

[Thomas Huth]

The libudev-related code is independent from the other pci-related code
and can be re-used for non-pci devices (like ccw devices on s390x). Thus
move this part to the generic function.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1755075
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qga/commands-posix.c | 62 +++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1a42ec8171..e8467ac567 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -874,10 +874,6 @@ static bool build_guest_fsinfo_for_pci_dev(char const *syspath,
     GuestPCIAddress *pciaddr = disk->pci_controller;
     bool has_ata = false, has_host = false, has_tgt = false;
     char *p, *q, *driver = NULL;
-#ifdef CONFIG_LIBUDEV
-    struct udev *udev = NULL;
-    struct udev_device *udevice = NULL;
-#endif
     bool ret = false;
 
     p = strstr(syspath, "/devices/pci");
@@ -936,26 +932,6 @@ static bool build_guest_fsinfo_for_pci_dev(char const *syspath,
     pciaddr->slot = pci[2];
     pciaddr->function = pci[3];
 
-#ifdef CONFIG_LIBUDEV
-    udev = udev_new();
-    udevice = udev_device_new_from_syspath(udev, syspath);
-    if (udev == NULL || udevice == NULL) {
-        g_debug("failed to query udev");
-    } else {
-        const char *devnode, *serial;
-        devnode = udev_device_get_devnode(udevice);
-        if (devnode != NULL) {
-            disk->dev = g_strdup(devnode);
-            disk->has_dev = true;
-        }
-        serial = udev_device_get_property_value(udevice, "ID_SERIAL");
-        if (serial != NULL && *serial != 0) {
-            disk->serial = g_strdup(serial);
-            disk->has_serial = true;
-        }
-    }
-#endif
-
     if (strcmp(driver, "ata_piix") == 0) {
         /* a host per ide bus, target*:0:<unit>:0 */
         if (!has_host || !has_tgt) {
@@ -1017,10 +993,6 @@ static bool build_guest_fsinfo_for_pci_dev(char const *syspath,
 
 cleanup:
     g_free(driver);
-#ifdef CONFIG_LIBUDEV
-    udev_unref(udev);
-    udev_device_unref(udevice);
-#endif
     return ret;
 }
 
@@ -1033,18 +1005,50 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
     GuestPCIAddress *pciaddr;
     GuestDiskAddressList *list = NULL;
     bool has_hwinf;
+#ifdef CONFIG_LIBUDEV
+    struct udev *udev = NULL;
+    struct udev_device *udevice = NULL;
+#endif
 
     pciaddr = g_new0(GuestPCIAddress, 1);
+    pciaddr->domain = -1;                       /* -1 means field is invalid */
+    pciaddr->bus = -1;
+    pciaddr->slot = -1;
+    pciaddr->function = -1;
 
     disk = g_new0(GuestDiskAddress, 1);
     disk->pci_controller = pciaddr;
+    disk->bus_type = GUEST_DISK_BUS_TYPE_UNKNOWN;
 
     list = g_new0(GuestDiskAddressList, 1);
     list->value = disk;
 
+#ifdef CONFIG_LIBUDEV
+    udev = udev_new();
+    udevice = udev_device_new_from_syspath(udev, syspath);
+    if (udev == NULL || udevice == NULL) {
+        g_debug("failed to query udev");
+    } else {
+        const char *devnode, *serial;
+        devnode = udev_device_get_devnode(udevice);
+        if (devnode != NULL) {
+            disk->dev = g_strdup(devnode);
+            disk->has_dev = true;
+        }
+        serial = udev_device_get_property_value(udevice, "ID_SERIAL");
+        if (serial != NULL && *serial != 0) {
+            disk->serial = g_strdup(serial);
+            disk->has_serial = true;
+        }
+    }
+
+    udev_unref(udev);
+    udev_device_unref(udevice);
+#endif
+
     has_hwinf = build_guest_fsinfo_for_pci_dev(syspath, disk, errp);
 
-    if (has_hwinf) {
+    if (has_hwinf || disk->has_dev || disk->has_serial) {
         list->next = fs->disk;
         fs->disk = list;
     } else {
-- 
2.18.1

[PATCH v2 4/4] qga/commands-posix: Support fsinfo for non-PCI virtio devices, too

[Thomas Huth]

QEMU on s390x uses virtio via channel I/O instead of PCI by default.
Add a function to detect and provide information for virtio-scsi and
virtio-block devices here, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qga/commands-posix.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index e8467ac567..744c2b5a5d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -996,6 +996,39 @@ cleanup:
     return ret;
 }
 
+/*
+ * Store disk device info for non-PCI virtio devices (for example s390x
+ * channel I/O devices). Returns true if information has been stored, or
+ * false for failure.
+ */
+static bool build_guest_fsinfo_for_nonpci_virtio(char const *syspath,
+                                                 GuestDiskAddress *disk,
+                                                 Error **errp)
+{
+    unsigned int tgt[3];
+    char *p;
+
+    if (!strstr(syspath, "/virtio") || !strstr(syspath, "/block")) {
+        g_debug("Unsupported virtio device '%s'", syspath);
+        return false;
+    }
+
+    p = strstr(syspath, "/target");
+    if (p && sscanf(p + 7, "%*u:%*u:%*u/%*u:%u:%u:%u",
+                    &tgt[0], &tgt[1], &tgt[2]) == 3) {
+        /* virtio-scsi: target*:0:<target>:<unit> */
+        disk->bus_type = GUEST_DISK_BUS_TYPE_SCSI;
+        disk->bus = tgt[0];
+        disk->target = tgt[1];
+        disk->unit = tgt[2];
+    } else {
+        /* virtio-blk: 1 disk per 1 device */
+        disk->bus_type = GUEST_DISK_BUS_TYPE_VIRTIO;
+    }
+
+    return true;
+}
+
 /* Store disk device info specified by @sysfs into @fs */
 static void build_guest_fsinfo_for_real_device(char const *syspath,
                                                GuestFilesystemInfo *fs,
@@ -1046,7 +1079,14 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
     udev_device_unref(udevice);
 #endif
 
-    has_hwinf = build_guest_fsinfo_for_pci_dev(syspath, disk, errp);
+    if (strstr(syspath, "/devices/pci")) {
+        has_hwinf = build_guest_fsinfo_for_pci_dev(syspath, disk, errp);
+    } else if (strstr(syspath, "/virtio")) {
+        has_hwinf = build_guest_fsinfo_for_nonpci_virtio(syspath, disk, errp);
+    } else {
+        g_debug("Unsupported device type for '%s'", syspath);
+        has_hwinf = false;
+    }
 
     if (has_hwinf || disk->has_dev || disk->has_serial) {
         list->next = fs->disk;
-- 
2.18.1

Re: [PATCH v2 4/4] qga/commands-posix: Support fsinfo for non-PCI virtio devices, too

[Daniel P. Berrangé]

On Wed, Jul 22, 2020 at 06:40:28AM +0200, Thomas Huth wrote:
> QEMU on s390x uses virtio via channel I/O instead of PCI by default.
> Add a function to detect and provide information for virtio-scsi and
> virtio-block devices here, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  qga/commands-posix.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 3/4] qga/commands-posix: Move the udev code from the pci to the generic function

[Daniel P. Berrangé]

On Wed, Jul 22, 2020 at 06:40:27AM +0200, Thomas Huth wrote:
> The libudev-related code is independent from the other pci-related code
> and can be re-used for non-pci devices (like ccw devices on s390x). Thus
> move this part to the generic function.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1755075
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  qga/commands-posix.c | 62 +++++++++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 29 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/4] qga/commands-posix: Rework build_guest_fsinfo_for_real_device() function

[Daniel P. Berrangé]

On Wed, Jul 22, 2020 at 06:40:26AM +0200, Thomas Huth wrote:
> We are going to support non-PCI devices soon. For this we need to split
> the generic GuestDiskAddress and GuestDiskAddressList memory allocation
> and list chaining into a separate function first.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  qga/commands-posix.c | 65 ++++++++++++++++++++++++++++----------------
>  1 file changed, 41 insertions(+), 24 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 0/4] Allow guest-get-fsinfo also for non-PCI devices

[Michael Roth]

Quoting Thomas Huth (2020-07-21 23:40:24)
> The information that can be retrieved via UDEV is also usable for non-PCI
> devices. So let's allow build_guest_fsinfo_for_real_device() on non-PCI
> devices, too. This is required to fix the bug that CCW devices show up
> without "Target" when running libvirt's "virsh domfsinfo" command (see
> https://bugzilla.redhat.com/show_bug.cgi?id=1755075 for details).
> 
> v2:
>  - Use g_new0 instead of g_malloc0 (as suggested by Daniel)
>  - Init fields to -1 explicitely, not via memset (Daniel)
>  - Add the fourth patch to also fill in virtio information on s390x

Thanks, patches 2-4 applied to qga-staging tree for 5.2:
  https://github.com/mdroth/qemu/commits/qga-staging

I've sent a pull request for 5.1 with patch 1/4

> 
> Thomas Huth (4):
>   qga/qapi-schema: Document -1 for invalid PCI address fields
>   qga/commands-posix: Rework build_guest_fsinfo_for_real_device()
>     function
>   qga/commands-posix: Move the udev code from the pci to the generic
>     function
>   qga/commands-posix: Support fsinfo for non-PCI virtio devices, too
> 
>  qga/commands-posix.c | 157 ++++++++++++++++++++++++++++++-------------
>  qga/qapi-schema.json |   2 +-
>  2 files changed, 110 insertions(+), 49 deletions(-)
> 
> -- 
> 2.18.1
>