[PATCH for-5.2 0/3] Allow guest-get-fsinfo also for non-PCI devices

[PATCH for-5.2 0/3] 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).

Thomas Huth (3):
  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.c | 113 +++++++++++++++++++++++++------------------
 qga/qapi-schema.json |   2 +-
 2 files changed, 66 insertions(+), 49 deletions(-)

-- 
2.18.1

[PATCH for-5.2 1/3] 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.

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 for-5.2 2/3] 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 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..cddbaf5c69 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,
+                                           GuestPCIAddress *pciaddr,
+                                           Error **errp)
 {
     unsigned int pci[4], host, hosts[8], tgt[3];
     int i, nhosts = 0, pcilen;
-    GuestDiskAddress *disk;
-    GuestPCIAddress *pciaddr;
-    GuestDiskAddressList *list = NULL;
     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_pci;
+
+    pciaddr = g_malloc(sizeof(*pciaddr));
+    memset(pciaddr, -1, sizeof(*pciaddr));  /* -1 means field is invalid */
+
+    disk = g_malloc0(sizeof(*disk));
+    disk->pci_controller = pciaddr;
+
+    list = g_malloc0(sizeof(*list));
+    list->value = disk;
+
+    has_pci = build_guest_fsinfo_for_pci_dev(syspath, disk, pciaddr, errp);
+    if (has_pci) {
+        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 for-5.2 3/3] 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 | 58 ++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index cddbaf5c69..169cb9195c 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,
     int i, nhosts = 0, pcilen;
     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,46 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
     GuestPCIAddress *pciaddr;
     GuestDiskAddressList *list = NULL;
     bool has_pci;
+#ifdef CONFIG_LIBUDEV
+    struct udev *udev = NULL;
+    struct udev_device *udevice = NULL;
+#endif
 
     pciaddr = g_malloc(sizeof(*pciaddr));
     memset(pciaddr, -1, sizeof(*pciaddr));  /* -1 means field is invalid */
 
     disk = g_malloc0(sizeof(*disk));
     disk->pci_controller = pciaddr;
+    disk->bus_type = GUEST_DISK_BUS_TYPE_UNKNOWN;
 
     list = g_malloc0(sizeof(*list));
     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_pci = build_guest_fsinfo_for_pci_dev(syspath, disk, pciaddr, errp);
-    if (has_pci) {
+    if (has_pci || disk->has_dev || disk->has_serial) {
         list->next = fs->disk;
         fs->disk = list;
     } else {
-- 
2.18.1

Re: [PATCH for-5.2 1/3] qga/qapi-schema: Document -1 for invalid PCI address fields

[Daniel P. Berrangé]

On Mon, Jul 20, 2020 at 01:01:31PM +0200, Thomas Huth wrote:
> 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.

Urgh that's a bit of a design flaw - "pci-controller" should have
been an optional field instead, but we can't fix that without
breaking compat with existing clients :-(

> 
> 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

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 for-5.2 2/3] qga/commands-posix: Rework build_guest_fsinfo_for_real_device() function

[Daniel P. Berrangé]

On Mon, Jul 20, 2020 at 01:01:32PM +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 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..cddbaf5c69 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,
> +                                           GuestPCIAddress *pciaddr,
> +                                           Error **errp)
>  {
>      unsigned int pci[4], host, hosts[8], tgt[3];
>      int i, nhosts = 0, pcilen;
> -    GuestDiskAddress *disk;
> -    GuestPCIAddress *pciaddr;
> -    GuestDiskAddressList *list = NULL;
>      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_pci;
> +
> +    pciaddr = g_malloc(sizeof(*pciaddr));

g_new0 instead of g_malloc and thus kill the sizeof.

> +    memset(pciaddr, -1, sizeof(*pciaddr));  /* -1 means field is invalid */

Each field in GuestPCIAddress is an "int64_t", but memset works on bytes.

So you're not setting the fields to "-1" here, you're setting
each octet in the "int64_t" to -1.

> +
> +    disk = g_malloc0(sizeof(*disk));
> +    disk->pci_controller = pciaddr;
> +
> +    list = g_malloc0(sizeof(*list));
> +    list->value = disk;

g_new0 for these too.

(yes, I realize these were all pre-existing bugs)

> +
> +    has_pci = build_guest_fsinfo_for_pci_dev(syspath, disk, pciaddr, errp);
> +    if (has_pci) {
> +        list->next = fs->disk;
> +        fs->disk = list;
> +    } else {
> +        qapi_free_GuestDiskAddressList(list);
> +    }
>  }


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 for-5.2 3/3] qga/commands-posix: Move the udev code from the pci to the generic function

[Daniel P. Berrangé]

On Mon, Jul 20, 2020 at 01:01:33PM +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 | 58 ++++++++++++++++++++++----------------------
>  1 file changed, 29 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 :|