| Message ID | 20201021071520.2168877-2-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qga: Fix several guest-get-devices issues | expand |
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(-)
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 >
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!
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;
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(-)