* [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
* [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
* [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 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
* 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
* 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.