[PATCH 0/4] qga: Fix several guest-get-devices issues

[PATCH 0/4] qga: Fix several guest-get-devices issues

[Markus Armbruster]

To be frank, I'm disappointed the code passed review in this state.
There are excuses for each of the issues addressed in this series
(PATCH 1 naming is hard, PATCH 2 recognizing non-local relations is
hard, PATCH 3 mistakes on error paths are easy to miss, PATCH 4 can't
expect all reviewers to know "no more simple unions, please").  Still,
having so many of them escape notice is disappointing.

Please review.

Markus Armbruster (4):
  qga: Rename guest-get-devices return member 'address' to 'id'
  qga: Use common time encoding for guest-get-devices 'driver-date'
  qga-win: Fix guest-get-devices error API violations
  qga: Flatten simple union GuestDeviceId

 qga/qapi-schema.json | 29 +++++++++++++++++++----------
 qga/commands-win32.c | 39 ++++++++++++++++++++-------------------
 2 files changed, 39 insertions(+), 29 deletions(-)

-- 
2.26.2

[PATCH 1/4] qga: Rename guest-get-devices return member 'address' to 'id'

[Markus Armbruster]

Member 'address' is union GuestDeviceAddress with a single branch
GuestDeviceAddressPCI, containing PCI vendor ID and device ID.  This
is not a PCI address.  Type GuestPCIAddress is.  Messed up in recent
commit 2e4211cee4 "qga: add command guest-get-devices for reporting
VirtIO devices".

Rename type GuestDeviceAddressPCI to GuestDeviceIdPCI, type
GuestDeviceAddress to GuestDeviceId, and member 'address' to 'id'.

Document the member properly while there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/qapi-schema.json | 17 +++++++++--------
 qga/commands-win32.c | 16 ++++++++--------
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index cec98c7e06..f2c81cda2b 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1257,26 +1257,26 @@
   'returns': 'GuestOSInfo' }
 
 ##
-# @GuestDeviceAddressPCI:
+# @GuestDeviceIdPCI:
 #
 # @vendor-id: vendor ID
 # @device-id: device ID
 #
 # Since: 5.2
 ##
-{ 'struct': 'GuestDeviceAddressPCI',
+{ 'struct': 'GuestDeviceIdPCI',
   'data': { 'vendor-id': 'uint16', 'device-id': 'uint16' } }
 
 ##
-# @GuestDeviceAddress:
+# @GuestDeviceId:
 #
-# Address of the device
-# - @pci: address of PCI device, since: 5.2
+# Id of the device
+# - @pci: PCI ID, since: 5.2
 #
 # Since: 5.2
 ##
-{ 'union': 'GuestDeviceAddress',
-  'data': { 'pci': 'GuestDeviceAddressPCI' } }
+{ 'union': 'GuestDeviceId',
+  'data': { 'pci': 'GuestDeviceIdPCI' } }
 
 ##
 # @GuestDeviceInfo:
@@ -1284,6 +1284,7 @@
 # @driver-name: name of the associated driver
 # @driver-date: driver release date in format YYYY-MM-DD
 # @driver-version: driver version
+# @id: device ID
 #
 # Since: 5.2
 ##
@@ -1292,7 +1293,7 @@
       'driver-name': 'str',
       '*driver-date': 'str',
       '*driver-version': 'str',
-      '*address': 'GuestDeviceAddress'
+      '*id': 'GuestDeviceId'
   } }
 
 ##
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0c3c05484f..879b02b6c3 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2390,22 +2390,22 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
         }
         for (j = 0; hw_ids[j] != NULL; j++) {
             GMatchInfo *match_info;
-            GuestDeviceAddressPCI *address;
+            GuestDeviceIdPCI *id;
             if (!g_regex_match(device_pci_re, hw_ids[j], 0, &match_info)) {
                 continue;
             }
             skip = false;
 
-            address = g_new0(GuestDeviceAddressPCI, 1);
+            id = g_new0(GuestDeviceIdPCI, 1);
             vendor_id = g_match_info_fetch(match_info, 1);
             device_id = g_match_info_fetch(match_info, 2);
-            address->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
-            address->device_id = g_ascii_strtoull(device_id, NULL, 16);
+            id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
+            id->device_id = g_ascii_strtoull(device_id, NULL, 16);
 
-            device->address = g_new0(GuestDeviceAddress, 1);
-            device->has_address = true;
-            device->address->type = GUEST_DEVICE_ADDRESS_KIND_PCI;
-            device->address->u.pci.data = address;
+            device->id = g_new0(GuestDeviceId, 1);
+            device->has_id = true;
+            device->id->type = GUEST_DEVICE_ID_KIND_PCI;
+            device->id->u.pci.data = id;
 
             g_match_info_free(match_info);
             break;
-- 
2.26.2

Re: [PATCH 1/4] qga: Rename guest-get-devices return member 'address' to 'id'

[Philippe Mathieu-Daudé]

On 10/21/20 9:15 AM, Markus Armbruster wrote:
> Member 'address' is union GuestDeviceAddress with a single branch
> GuestDeviceAddressPCI, containing PCI vendor ID and device ID.  This
> is not a PCI address.  Type GuestPCIAddress is.  Messed up in recent
> commit 2e4211cee4 "qga: add command guest-get-devices for reporting
> VirtIO devices".

Out of curiosity, how did you notice?

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Rename type GuestDeviceAddressPCI to GuestDeviceIdPCI, type
> GuestDeviceAddress to GuestDeviceId, and member 'address' to 'id'.
> 
> Document the member properly while there.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qga/qapi-schema.json | 17 +++++++++--------
>   qga/commands-win32.c | 16 ++++++++--------
>   2 files changed, 17 insertions(+), 16 deletions(-)

Re: [PATCH 1/4] qga: Rename guest-get-devices return member 'address' to 'id'

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 10/21/20 9:15 AM, Markus Armbruster wrote:
>> Member 'address' is union GuestDeviceAddress with a single branch
>> GuestDeviceAddressPCI, containing PCI vendor ID and device ID.  This
>> is not a PCI address.  Type GuestPCIAddress is.  Messed up in recent
>> commit 2e4211cee4 "qga: add command guest-get-devices for reporting
>> VirtIO devices".
>
> Out of curiosity, how did you notice?

I searched for simple unions to remind myself of the size of that
problem, and found a new one, i.e. a problem I can still avoid.  I
started to do PATCH 4, and noticed the other issues.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

Re: [PATCH 1/4] qga: Rename guest-get-devices return member 'address' to 'id'

[Marc-André Lureau]

On Wed, Oct 21, 2020 at 11:15 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Member 'address' is union GuestDeviceAddress with a single branch
> GuestDeviceAddressPCI, containing PCI vendor ID and device ID.  This
> is not a PCI address.  Type GuestPCIAddress is.  Messed up in recent
> commit 2e4211cee4 "qga: add command guest-get-devices for reporting
> VirtIO devices".
>
> Rename type GuestDeviceAddressPCI to GuestDeviceIdPCI, type
> GuestDeviceAddress to GuestDeviceId, and member 'address' to 'id'.
>
> Document the member properly while there.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  qga/qapi-schema.json | 17 +++++++++--------
>  qga/commands-win32.c | 16 ++++++++--------
>  2 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index cec98c7e06..f2c81cda2b 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1257,26 +1257,26 @@
>    'returns': 'GuestOSInfo' }
>
>  ##
> -# @GuestDeviceAddressPCI:
> +# @GuestDeviceIdPCI:
>  #
>  # @vendor-id: vendor ID
>  # @device-id: device ID
>  #
>  # Since: 5.2
>  ##
> -{ 'struct': 'GuestDeviceAddressPCI',
> +{ 'struct': 'GuestDeviceIdPCI',
>    'data': { 'vendor-id': 'uint16', 'device-id': 'uint16' } }
>
>  ##
> -# @GuestDeviceAddress:
> +# @GuestDeviceId:
>  #
> -# Address of the device
> -# - @pci: address of PCI device, since: 5.2
> +# Id of the device
> +# - @pci: PCI ID, since: 5.2
>  #
>  # Since: 5.2
>  ##
> -{ 'union': 'GuestDeviceAddress',
> -  'data': { 'pci': 'GuestDeviceAddressPCI' } }
> +{ 'union': 'GuestDeviceId',
> +  'data': { 'pci': 'GuestDeviceIdPCI' } }
>
>  ##
>  # @GuestDeviceInfo:
> @@ -1284,6 +1284,7 @@
>  # @driver-name: name of the associated driver
>  # @driver-date: driver release date in format YYYY-MM-DD
>  # @driver-version: driver version
> +# @id: device ID
>  #
>  # Since: 5.2
>  ##
> @@ -1292,7 +1293,7 @@
>        'driver-name': 'str',
>        '*driver-date': 'str',
>        '*driver-version': 'str',
> -      '*address': 'GuestDeviceAddress'
> +      '*id': 'GuestDeviceId'
>    } }
>
>  ##
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 0c3c05484f..879b02b6c3 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -2390,22 +2390,22 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
>          }
>          for (j = 0; hw_ids[j] != NULL; j++) {
>              GMatchInfo *match_info;
> -            GuestDeviceAddressPCI *address;
> +            GuestDeviceIdPCI *id;
>              if (!g_regex_match(device_pci_re, hw_ids[j], 0, &match_info)) {
>                  continue;
>              }
>              skip = false;
>
> -            address = g_new0(GuestDeviceAddressPCI, 1);
> +            id = g_new0(GuestDeviceIdPCI, 1);
>              vendor_id = g_match_info_fetch(match_info, 1);
>              device_id = g_match_info_fetch(match_info, 2);
> -            address->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
> -            address->device_id = g_ascii_strtoull(device_id, NULL, 16);
> +            id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
> +            id->device_id = g_ascii_strtoull(device_id, NULL, 16);
>
> -            device->address = g_new0(GuestDeviceAddress, 1);
> -            device->has_address = true;
> -            device->address->type = GUEST_DEVICE_ADDRESS_KIND_PCI;
> -            device->address->u.pci.data = address;
> +            device->id = g_new0(GuestDeviceId, 1);
> +            device->has_id = true;
> +            device->id->type = GUEST_DEVICE_ID_KIND_PCI;
> +            device->id->u.pci.data = id;
>
>              g_match_info_free(match_info);
>              break;
> --
> 2.26.2
>

[PATCH 2/4] qga: Use common time encoding for guest-get-devices 'driver-date'

[Markus Armbruster]

guest-get-devices returns 'driver-date' as string in the format
YYYY-MM-DD.  Goes back to recent commit 2e4211cee4 "qga: add command
guest-get-devices for reporting VirtIO devices".

We should avoid use of multiple encodings for the same kind of data.
Especially string encodings.  Change it to return nanoseconds since
the epoch, like guest-get-time does.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/qapi-schema.json |  4 ++--
 qga/commands-win32.c | 19 +++++++++++--------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index f2c81cda2b..c7bfb8bf6a 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1282,7 +1282,7 @@
 # @GuestDeviceInfo:
 #
 # @driver-name: name of the associated driver
-# @driver-date: driver release date in format YYYY-MM-DD
+# @driver-date: driver release date, in nanoseconds since the epoch
 # @driver-version: driver version
 # @id: device ID
 #
@@ -1291,7 +1291,7 @@
 { 'struct': 'GuestDeviceInfo',
   'data': {
       'driver-name': 'str',
-      '*driver-date': 'str',
+      '*driver-date': 'int',
       '*driver-version': 'str',
       '*id': 'GuestDeviceId'
   } }
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 879b02b6c3..b01616a992 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1641,6 +1641,12 @@ out:
     return head;
 }
 
+static int64_t filetime_to_ns(const FILETIME *tf)
+{
+    return ((((int64_t)tf->dwHighDateTime << 32) | tf->dwLowDateTime)
+            - W32_FT_OFFSET) * 100;
+}
+
 int64_t qmp_guest_get_time(Error **errp)
 {
     SYSTEMTIME ts = {0};
@@ -1657,8 +1663,7 @@ int64_t qmp_guest_get_time(Error **errp)
         return -1;
     }
 
-    return ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
-                - W32_FT_OFFSET) * 100;
+    return filetime_to_ns(&tf);
 }
 
 void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
@@ -2363,7 +2368,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
     slog("enumerating devices");
     for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
         bool skip = true;
-        SYSTEMTIME utc_date;
         g_autofree LPWSTR name = NULL;
         g_autofree LPFILETIME date = NULL;
         g_autofree LPWSTR version = NULL;
@@ -2434,13 +2438,12 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
             slog("failed to get driver date");
             continue;
         }
-        FileTimeToSystemTime(date, &utc_date);
-        device->driver_date = g_strdup_printf("%04d-%02d-%02d",
-            utc_date.wYear, utc_date.wMonth, utc_date.wDay);
+        device->driver_date = filetime_to_ns(date);
         device->has_driver_date = true;
 
-        slog("driver: %s\ndriver version: %s,%s\n", device->driver_name,
-            device->driver_date, device->driver_version);
+        slog("driver: %s\ndriver version: %" PRId64 ",%s\n",
+             device->driver_name, device->driver_date,
+             device->driver_version);
         item = g_new0(GuestDeviceInfoList, 1);
         item->value = g_steal_pointer(&device);
         if (!cur_item) {
-- 
2.26.2

Re: [PATCH 2/4] qga: Use common time encoding for guest-get-devices 'driver-date'

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3790 bytes --]

Hi

On Wed, Oct 21, 2020 at 11:18 AM Markus Armbruster <armbru@redhat.com>
wrote:

> guest-get-devices returns 'driver-date' as string in the format
> YYYY-MM-DD.  Goes back to recent commit 2e4211cee4 "qga: add command
> guest-get-devices for reporting VirtIO devices".
>
> We should avoid use of multiple encodings for the same kind of data.
> Especially string encodings.  Change it to return nanoseconds since
> the epoch, like guest-get-time does.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>

I think I assumed it was free form human-friendly string, but it has a
strict YYYY-MM-DD format in the doc, so let's use the existing one instead.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



> ---
>  qga/qapi-schema.json |  4 ++--
>  qga/commands-win32.c | 19 +++++++++++--------
>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index f2c81cda2b..c7bfb8bf6a 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1282,7 +1282,7 @@
>  # @GuestDeviceInfo:
>  #
>  # @driver-name: name of the associated driver
> -# @driver-date: driver release date in format YYYY-MM-DD
> +# @driver-date: driver release date, in nanoseconds since the epoch
>  # @driver-version: driver version
>  # @id: device ID
>  #
> @@ -1291,7 +1291,7 @@
>  { 'struct': 'GuestDeviceInfo',
>    'data': {
>        'driver-name': 'str',
> -      '*driver-date': 'str',
> +      '*driver-date': 'int',
>        '*driver-version': 'str',
>        '*id': 'GuestDeviceId'
>    } }
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 879b02b6c3..b01616a992 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -1641,6 +1641,12 @@ out:
>      return head;
>  }
>
> +static int64_t filetime_to_ns(const FILETIME *tf)
> +{
> +    return ((((int64_t)tf->dwHighDateTime << 32) | tf->dwLowDateTime)
> +            - W32_FT_OFFSET) * 100;
> +}
> +
>  int64_t qmp_guest_get_time(Error **errp)
>  {
>      SYSTEMTIME ts = {0};
> @@ -1657,8 +1663,7 @@ int64_t qmp_guest_get_time(Error **errp)
>          return -1;
>      }
>
> -    return ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
> -                - W32_FT_OFFSET) * 100;
> +    return filetime_to_ns(&tf);
>  }
>
>  void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
> @@ -2363,7 +2368,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error
> **errp)
>      slog("enumerating devices");
>      for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) {
>          bool skip = true;
> -        SYSTEMTIME utc_date;
>          g_autofree LPWSTR name = NULL;
>          g_autofree LPFILETIME date = NULL;
>          g_autofree LPWSTR version = NULL;
> @@ -2434,13 +2438,12 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error
> **errp)
>              slog("failed to get driver date");
>              continue;
>          }
> -        FileTimeToSystemTime(date, &utc_date);
> -        device->driver_date = g_strdup_printf("%04d-%02d-%02d",
> -            utc_date.wYear, utc_date.wMonth, utc_date.wDay);
> +        device->driver_date = filetime_to_ns(date);
>          device->has_driver_date = true;
>
> -        slog("driver: %s\ndriver version: %s,%s\n", device->driver_name,
> -            device->driver_date, device->driver_version);
> +        slog("driver: %s\ndriver version: %" PRId64 ",%s\n",
> +             device->driver_name, device->driver_date,
> +             device->driver_version);
>          item = g_new0(GuestDeviceInfoList, 1);
>          item->value = g_steal_pointer(&device);
>          if (!cur_item) {
> --
> 2.26.2
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 5087 bytes --]

[PATCH 3/4] qga-win: Fix guest-get-devices error API violations

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

qmp_guest_get_devices() is wrong that way: it calls error_setg() in a
loop.

If no iteration fails, the function returns a value and sets no error.
Okay.

If exactly one iteration fails, the function returns a value and sets
an error.  Wrong.

If multiple iterations fail, the function trips error_setv()'s
assertion.

Fix it to return immediately on error.

Perhaps the failure to convert the driver version to UTF-8 should not
be an error.  We could simply not report the botched version string
instead.

Drop a superfluous continue while there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands-win32.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b01616a992..1efe3ba076 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2385,7 +2385,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
         device->driver_name = g_utf16_to_utf8(name, -1, NULL, NULL, NULL);
         if (device->driver_name == NULL) {
             error_setg(errp, "conversion to utf8 failed (driver name)");
-            continue;
+            return NULL;
         }
         slog("querying device: %s", device->driver_name);
         hw_ids = ga_get_hardware_ids(dev_info_data.DevInst);
@@ -2428,7 +2428,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
             NULL, NULL);
         if (device->driver_version == NULL) {
             error_setg(errp, "conversion to utf8 failed (driver version)");
-            continue;
+            return NULL;
         }
         device->has_driver_version = true;
 
@@ -2452,7 +2452,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
             cur_item->next = item;
             cur_item = item;
         }
-        continue;
     }
 
     if (dev_info != INVALID_HANDLE_VALUE) {
-- 
2.26.2

Re: [PATCH 3/4] qga-win: Fix guest-get-devices error API violations

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2519 bytes --]

On Wed, Oct 21, 2020 at 11:19 AM Markus Armbruster <armbru@redhat.com>
wrote:

> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
>
> qmp_guest_get_devices() is wrong that way: it calls error_setg() in a
> loop.
>
> If no iteration fails, the function returns a value and sets no error.
> Okay.
>
> If exactly one iteration fails, the function returns a value and sets
> an error.  Wrong.
>
> If multiple iterations fail, the function trips error_setv()'s
> assertion.
>
> Fix it to return immediately on error.
>
> Perhaps the failure to convert the driver version to UTF-8 should not
> be an error.  We could simply not report the botched version string
> instead.
>
> Drop a superfluous continue while there.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  qga/commands-win32.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index b01616a992..1efe3ba076 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -2385,7 +2385,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error
> **errp)
>          device->driver_name = g_utf16_to_utf8(name, -1, NULL, NULL, NULL);
>          if (device->driver_name == NULL) {
>              error_setg(errp, "conversion to utf8 failed (driver name)");
> -            continue;
> +            return NULL;
>          }
>          slog("querying device: %s", device->driver_name);
>          hw_ids = ga_get_hardware_ids(dev_info_data.DevInst);
> @@ -2428,7 +2428,7 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error
> **errp)
>              NULL, NULL);
>          if (device->driver_version == NULL) {
>              error_setg(errp, "conversion to utf8 failed (driver
> version)");
> -            continue;
> +            return NULL;
>          }
>          device->has_driver_version = true;
>
> @@ -2452,7 +2452,6 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error
> **errp)
>              cur_item->next = item;
>              cur_item = item;
>          }
> -        continue;
>      }
>
>      if (dev_info != INVALID_HANDLE_VALUE) {
> --
> 2.26.2
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3485 bytes --]

Re: [PATCH 3/4] qga-win: Fix guest-get-devices error API violations

[Philippe Mathieu-Daudé]

On 10/21/20 9:15 AM, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
> 
> qmp_guest_get_devices() is wrong that way: it calls error_setg() in a
> loop.
> 
> If no iteration fails, the function returns a value and sets no error.
> Okay.
> 
> If exactly one iteration fails, the function returns a value and sets
> an error.  Wrong.
> 
> If multiple iterations fail, the function trips error_setv()'s
> assertion.
> 
> Fix it to return immediately on error.
> 
> Perhaps the failure to convert the driver version to UTF-8 should not
> be an error.  We could simply not report the botched version string
> instead.
> 
> Drop a superfluous continue while there.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qga/commands-win32.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 4/4] qga: Flatten simple union GuestDeviceId

[Markus Armbruster]

Simple unions are simpler than flat unions in the schema, but more
complicated in C and on the QMP wire: there's extra indirection in C
and extra nesting on the wire, both pointless.  They should be avoided
in new code.

GuestDeviceId was recently added for guest-get-devices.  Convert it to
a flat union.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/qapi-schema.json | 8 ++++++++
 qga/commands-win32.c | 9 ++++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index c7bfb8bf6a..fe10631e4c 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1256,6 +1256,12 @@
 { 'command': 'guest-get-osinfo',
   'returns': 'GuestOSInfo' }
 
+##
+# @GuestDeviceType:
+##
+{ 'enum': 'GuestDeviceType',
+  'data': [ 'pci' ] }
+
 ##
 # @GuestDeviceIdPCI:
 #
@@ -1276,6 +1282,8 @@
 # Since: 5.2
 ##
 { 'union': 'GuestDeviceId',
+  'base': { 'type': 'GuestDeviceType' },
+  'discriminator': 'type',
   'data': { 'pci': 'GuestDeviceIdPCI' } }
 
 ##
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 1efe3ba076..0c33d48aaa 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2400,16 +2400,15 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
             }
             skip = false;
 
-            id = g_new0(GuestDeviceIdPCI, 1);
             vendor_id = g_match_info_fetch(match_info, 1);
             device_id = g_match_info_fetch(match_info, 2);
-            id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
-            id->device_id = g_ascii_strtoull(device_id, NULL, 16);
 
             device->id = g_new0(GuestDeviceId, 1);
             device->has_id = true;
-            device->id->type = GUEST_DEVICE_ID_KIND_PCI;
-            device->id->u.pci.data = id;
+            device->id->type = GUEST_DEVICE_TYPE_PCI;
+            id = &device->id->u.pci;
+            id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
+            id->device_id = g_ascii_strtoull(device_id, NULL, 16);
 
             g_match_info_free(match_info);
             break;
-- 
2.26.2

Re: [PATCH 4/4] qga: Flatten simple union GuestDeviceId

[Philippe Mathieu-Daudé]

On 10/21/20 9:15 AM, Markus Armbruster wrote:
> Simple unions are simpler than flat unions in the schema, but more
> complicated in C and on the QMP wire: there's extra indirection in C
> and extra nesting on the wire, both pointless.  They should be avoided
> in new code.
> 
> GuestDeviceId was recently added for guest-get-devices.  Convert it to
> a flat union.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qga/qapi-schema.json | 8 ++++++++
>   qga/commands-win32.c | 9 ++++-----
>   2 files changed, 12 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 4/4] qga: Flatten simple union GuestDeviceId

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2471 bytes --]

On Wed, Oct 21, 2020 at 11:16 AM Markus Armbruster <armbru@redhat.com>
wrote:

> Simple unions are simpler than flat unions in the schema, but more
> complicated in C and on the QMP wire: there's extra indirection in C
> and extra nesting on the wire, both pointless.  They should be avoided
> in new code.
>
> GuestDeviceId was recently added for guest-get-devices.  Convert it to
> a flat union.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  qga/qapi-schema.json | 8 ++++++++
>  qga/commands-win32.c | 9 ++++-----
>  2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index c7bfb8bf6a..fe10631e4c 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1256,6 +1256,12 @@
>  { 'command': 'guest-get-osinfo',
>    'returns': 'GuestOSInfo' }
>
> +##
> +# @GuestDeviceType:
> +##
> +{ 'enum': 'GuestDeviceType',
> +  'data': [ 'pci' ] }
> +
>  ##
>  # @GuestDeviceIdPCI:
>  #
> @@ -1276,6 +1282,8 @@
>  # Since: 5.2
>  ##
>  { 'union': 'GuestDeviceId',
> +  'base': { 'type': 'GuestDeviceType' },
> +  'discriminator': 'type',
>    'data': { 'pci': 'GuestDeviceIdPCI' } }
>
>  ##
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 1efe3ba076..0c33d48aaa 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -2400,16 +2400,15 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error
> **errp)
>              }
>              skip = false;
>
> -            id = g_new0(GuestDeviceIdPCI, 1);
>              vendor_id = g_match_info_fetch(match_info, 1);
>              device_id = g_match_info_fetch(match_info, 2);
> -            id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
> -            id->device_id = g_ascii_strtoull(device_id, NULL, 16);
>
>              device->id = g_new0(GuestDeviceId, 1);
>              device->has_id = true;
> -            device->id->type = GUEST_DEVICE_ID_KIND_PCI;
> -            device->id->u.pci.data = id;
> +            device->id->type = GUEST_DEVICE_TYPE_PCI;
> +            id = &device->id->u.pci;
> +            id->vendor_id = g_ascii_strtoull(vendor_id, NULL, 16);
> +            id->device_id = g_ascii_strtoull(device_id, NULL, 16);
>
>              g_match_info_free(match_info);
>              break;
> --
> 2.26.2
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3591 bytes --]

Re: [PATCH 0/4] qga: Fix several guest-get-devices issues

[Markus Armbruster]

Mike,

This series addresses interface defects, and must go into 5.2.  It's
fully reviewed.  Pull request in time for soft freeze would be ideal.
If you can't make it, feel free to ask me.  I'll continue to push for
the series even if it misses the soft freeze.