* [PATCH for-5.2 0/3] Allow guest-get-fsinfo also for non-PCI devices
@ 2020-07-20 11:01 Thomas Huth
2020-07-20 11:01 ` [PATCH for-5.2 1/3] qga/qapi-schema: Document -1 for invalid PCI address fields Thomas Huth
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Thomas Huth @ 2020-07-20 11:01 UTC (permalink / raw)
To: qemu-devel, Michael Roth
Cc: Tomáš Golembiovský, Daniel P . Berrangé
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH for-5.2 1/3] qga/qapi-schema: Document -1 for invalid PCI address fields 2020-07-20 11:01 [PATCH for-5.2 0/3] Allow guest-get-fsinfo also for non-PCI devices Thomas Huth @ 2020-07-20 11:01 ` Thomas Huth 2020-07-21 8:51 ` Daniel P. Berrangé 2020-07-20 11:01 ` [PATCH for-5.2 2/3] qga/commands-posix: Rework build_guest_fsinfo_for_real_device() function Thomas Huth 2020-07-20 11:01 ` [PATCH for-5.2 3/3] qga/commands-posix: Move the udev code from the pci to the generic function Thomas Huth 2 siblings, 1 reply; 7+ messages in thread From: Thomas Huth @ 2020-07-20 11:01 UTC (permalink / raw) To: qemu-devel, Michael Roth Cc: Tomáš Golembiovský, Daniel P . Berrangé 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH for-5.2 1/3] qga/qapi-schema: Document -1 for invalid PCI address fields 2020-07-20 11:01 ` [PATCH for-5.2 1/3] qga/qapi-schema: Document -1 for invalid PCI address fields Thomas Huth @ 2020-07-21 8:51 ` Daniel P. Berrangé 0 siblings, 0 replies; 7+ messages in thread From: Daniel P. Berrangé @ 2020-07-21 8:51 UTC (permalink / raw) To: Thomas Huth; +Cc: Tomáš Golembiovský, qemu-devel, Michael Roth 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 :| ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH for-5.2 2/3] qga/commands-posix: Rework build_guest_fsinfo_for_real_device() function 2020-07-20 11:01 [PATCH for-5.2 0/3] Allow guest-get-fsinfo also for non-PCI devices Thomas Huth 2020-07-20 11:01 ` [PATCH for-5.2 1/3] qga/qapi-schema: Document -1 for invalid PCI address fields Thomas Huth @ 2020-07-20 11:01 ` Thomas Huth 2020-07-21 8:56 ` Daniel P. Berrangé 2020-07-20 11:01 ` [PATCH for-5.2 3/3] qga/commands-posix: Move the udev code from the pci to the generic function Thomas Huth 2 siblings, 1 reply; 7+ messages in thread From: Thomas Huth @ 2020-07-20 11:01 UTC (permalink / raw) To: qemu-devel, Michael Roth Cc: Tomáš Golembiovský, Daniel P . Berrangé 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH for-5.2 2/3] qga/commands-posix: Rework build_guest_fsinfo_for_real_device() function 2020-07-20 11:01 ` [PATCH for-5.2 2/3] qga/commands-posix: Rework build_guest_fsinfo_for_real_device() function Thomas Huth @ 2020-07-21 8:56 ` Daniel P. Berrangé 0 siblings, 0 replies; 7+ messages in thread From: Daniel P. Berrangé @ 2020-07-21 8:56 UTC (permalink / raw) To: Thomas Huth; +Cc: Tomáš Golembiovský, qemu-devel, Michael Roth 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 :| ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH for-5.2 3/3] qga/commands-posix: Move the udev code from the pci to the generic function 2020-07-20 11:01 [PATCH for-5.2 0/3] Allow guest-get-fsinfo also for non-PCI devices Thomas Huth 2020-07-20 11:01 ` [PATCH for-5.2 1/3] qga/qapi-schema: Document -1 for invalid PCI address fields Thomas Huth 2020-07-20 11:01 ` [PATCH for-5.2 2/3] qga/commands-posix: Rework build_guest_fsinfo_for_real_device() function Thomas Huth @ 2020-07-20 11:01 ` Thomas Huth 2020-07-21 8:58 ` Daniel P. Berrangé 2 siblings, 1 reply; 7+ messages in thread From: Thomas Huth @ 2020-07-20 11:01 UTC (permalink / raw) To: qemu-devel, Michael Roth Cc: Tomáš Golembiovský, Daniel P . Berrangé 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH for-5.2 3/3] qga/commands-posix: Move the udev code from the pci to the generic function 2020-07-20 11:01 ` [PATCH for-5.2 3/3] qga/commands-posix: Move the udev code from the pci to the generic function Thomas Huth @ 2020-07-21 8:58 ` Daniel P. Berrangé 0 siblings, 0 replies; 7+ messages in thread From: Daniel P. Berrangé @ 2020-07-21 8:58 UTC (permalink / raw) To: Thomas Huth; +Cc: Tomáš Golembiovský, qemu-devel, Michael Roth 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 :| ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-21 8:59 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-07-20 11:01 [PATCH for-5.2 0/3] Allow guest-get-fsinfo also for non-PCI devices Thomas Huth 2020-07-20 11:01 ` [PATCH for-5.2 1/3] qga/qapi-schema: Document -1 for invalid PCI address fields Thomas Huth 2020-07-21 8:51 ` Daniel P. Berrangé 2020-07-20 11:01 ` [PATCH for-5.2 2/3] qga/commands-posix: Rework build_guest_fsinfo_for_real_device() function Thomas Huth 2020-07-21 8:56 ` Daniel P. Berrangé 2020-07-20 11:01 ` [PATCH for-5.2 3/3] qga/commands-posix: Move the udev code from the pci to the generic function Thomas Huth 2020-07-21 8:58 ` Daniel P. Berrangé
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).