| Message ID | 1488435591-17882-6-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
Crash bug, I'm afraid... "Michael S. Tsirkin" <mst@redhat.com> writes: > From: Igor Mammedov <imammedo@redhat.com> > > Add commands to query Virtual Machine Generation ID counter. > > QMP command example: > { "execute": "query-vm-generation-id" } > > HMP command example: > info vm-generation-id > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Ben Warren <ben@skyportsystems.com> > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > Tested-by: Laszlo Ersek <lersek@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > qapi-schema.json | 20 ++++++++++++++++++++ > hmp.h | 1 + > hmp.c | 9 +++++++++ > hw/acpi/vmgenid.c | 16 ++++++++++++++++ > stubs/vmgenid.c | 9 +++++++++ > hmp-commands-info.hx | 14 ++++++++++++++ > stubs/Makefile.objs | 1 + > 7 files changed, 70 insertions(+) > create mode 100644 stubs/vmgenid.c > > diff --git a/qapi-schema.json b/qapi-schema.json > index d6186d4..84692da 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -6188,3 +6188,23 @@ > # > ## > { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] } > + > +## > +# @GuidInfo: > +# > +# GUID information. > +# > +# @guid: the globally unique identifier > +# > +# Since: 2.9 > +## > +{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} } > + > +## > +# @query-vm-generation-id: > +# > +# Show Virtual Machine Generation ID > +# > +# Since 2.9 > +## > +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } [...] > diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > index c8465df..744f284 100644 > --- a/hw/acpi/vmgenid.c > +++ b/hw/acpi/vmgenid.c > @@ -240,3 +240,19 @@ static void vmgenid_register_types(void) > } > > type_init(vmgenid_register_types) > + > +GuidInfo *qmp_query_vm_generation_id(Error **errp) > +{ > + GuidInfo *info; > + VmGenIdState *vms; > + Object *obj = find_vmgenid_dev(); > + > + if (!obj) { > + return NULL; > + } > + vms = VMGENID(obj); > + > + info = g_malloc0(sizeof(*info)); > + info->guid = qemu_uuid_unparse_strdup(&vms->guid); > + return info; > +} This either returns a GuidInfo or NULL. If it returns NULL, the generated qmp_marshal_output_GuidInfo() will crash like this: #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6 #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6 #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6 #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6 #4 0x0000555555c800a0 in visit_start_struct (v= 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49 #5 0x0000555555c5d398 in visit_type_GuidInfo (v= 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038 #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148 #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186 #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97 #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200) at /work/armbru/qemu/qapi/qmp-dispatch.c:124 #10 0x00005555557bb671 in handle_qmp_command (parser= 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789 #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1) at /work/armbru/qemu/qobject/json-streamer.c:105 Sorry for not having reviewed this earlier. On the other hand, how come this survived testing? [...]
On 03/02/17 09:11, Markus Armbruster wrote: > Crash bug, I'm afraid... > > "Michael S. Tsirkin" <mst@redhat.com> writes: > >> From: Igor Mammedov <imammedo@redhat.com> >> >> Add commands to query Virtual Machine Generation ID counter. >> >> QMP command example: >> { "execute": "query-vm-generation-id" } >> >> HMP command example: >> info vm-generation-id >> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: Ben Warren <ben@skyportsystems.com> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> Tested-by: Laszlo Ersek <lersek@redhat.com> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> --- >> qapi-schema.json | 20 ++++++++++++++++++++ >> hmp.h | 1 + >> hmp.c | 9 +++++++++ >> hw/acpi/vmgenid.c | 16 ++++++++++++++++ >> stubs/vmgenid.c | 9 +++++++++ >> hmp-commands-info.hx | 14 ++++++++++++++ >> stubs/Makefile.objs | 1 + >> 7 files changed, 70 insertions(+) >> create mode 100644 stubs/vmgenid.c >> >> diff --git a/qapi-schema.json b/qapi-schema.json >> index d6186d4..84692da 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -6188,3 +6188,23 @@ >> # >> ## >> { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] } >> + >> +## >> +# @GuidInfo: >> +# >> +# GUID information. >> +# >> +# @guid: the globally unique identifier >> +# >> +# Since: 2.9 >> +## >> +{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} } >> + >> +## >> +# @query-vm-generation-id: >> +# >> +# Show Virtual Machine Generation ID >> +# >> +# Since 2.9 >> +## >> +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } > [...] >> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c >> index c8465df..744f284 100644 >> --- a/hw/acpi/vmgenid.c >> +++ b/hw/acpi/vmgenid.c >> @@ -240,3 +240,19 @@ static void vmgenid_register_types(void) >> } >> >> type_init(vmgenid_register_types) >> + >> +GuidInfo *qmp_query_vm_generation_id(Error **errp) >> +{ >> + GuidInfo *info; >> + VmGenIdState *vms; >> + Object *obj = find_vmgenid_dev(); >> + >> + if (!obj) { >> + return NULL; >> + } >> + vms = VMGENID(obj); >> + >> + info = g_malloc0(sizeof(*info)); >> + info->guid = qemu_uuid_unparse_strdup(&vms->guid); >> + return info; >> +} > > This either returns a GuidInfo or NULL. > > If it returns NULL, the generated qmp_marshal_output_GuidInfo() will > crash like this: > > #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6 > #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6 > #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6 > #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6 > #4 0x0000555555c800a0 in visit_start_struct (v= > 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49 > #5 0x0000555555c5d398 in visit_type_GuidInfo (v= > 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038 > #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148 > #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186 > #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97 > #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200) > at /work/armbru/qemu/qapi/qmp-dispatch.c:124 > #10 0x00005555557bb671 in handle_qmp_command (parser= > 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789 > #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1) > at /work/armbru/qemu/qobject/json-streamer.c:105 > > Sorry for not having reviewed this earlier. On the other hand, how come > this survived testing? We primarily focused on testing the functionality, not the failure / corner cases, I think. (There are at least two other known startup scenarios that are not handled correctly just yet, although they don't crash -- multiple vmgenid devices, and vmgenid on machtypes without writeable fw_cfg.) Also, in my testing I only called the monitor command via HMP (from virsh), and AFAICS that one doesn't crash even if the device is missing. Nonetheless, qmp_query_vm_generation_id() should set an error when it returns NULL, yes. I think Ben wants to post a followup series with those fixes mentioned above, in time for 2.9, perhaps this crash can be addressed in there too. Ben? Thanks Laszlo
On 03/02/17 09:25, Laszlo Ersek wrote: > On 03/02/17 09:11, Markus Armbruster wrote: >> Crash bug, I'm afraid... >> >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >>> From: Igor Mammedov <imammedo@redhat.com> >>> >>> Add commands to query Virtual Machine Generation ID counter. >>> >>> QMP command example: >>> { "execute": "query-vm-generation-id" } >>> >>> HMP command example: >>> info vm-generation-id >>> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> Signed-off-by: Ben Warren <ben@skyportsystems.com> >>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>> Tested-by: Laszlo Ersek <lersek@redhat.com> >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> qapi-schema.json | 20 ++++++++++++++++++++ >>> hmp.h | 1 + >>> hmp.c | 9 +++++++++ >>> hw/acpi/vmgenid.c | 16 ++++++++++++++++ >>> stubs/vmgenid.c | 9 +++++++++ >>> hmp-commands-info.hx | 14 ++++++++++++++ >>> stubs/Makefile.objs | 1 + >>> 7 files changed, 70 insertions(+) >>> create mode 100644 stubs/vmgenid.c >>> >>> diff --git a/qapi-schema.json b/qapi-schema.json >>> index d6186d4..84692da 100644 >>> --- a/qapi-schema.json >>> +++ b/qapi-schema.json >>> @@ -6188,3 +6188,23 @@ >>> # >>> ## >>> { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] } >>> + >>> +## >>> +# @GuidInfo: >>> +# >>> +# GUID information. >>> +# >>> +# @guid: the globally unique identifier >>> +# >>> +# Since: 2.9 >>> +## >>> +{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} } >>> + >>> +## >>> +# @query-vm-generation-id: >>> +# >>> +# Show Virtual Machine Generation ID >>> +# >>> +# Since 2.9 >>> +## >>> +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } >> [...] >>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c >>> index c8465df..744f284 100644 >>> --- a/hw/acpi/vmgenid.c >>> +++ b/hw/acpi/vmgenid.c >>> @@ -240,3 +240,19 @@ static void vmgenid_register_types(void) >>> } >>> >>> type_init(vmgenid_register_types) >>> + >>> +GuidInfo *qmp_query_vm_generation_id(Error **errp) >>> +{ >>> + GuidInfo *info; >>> + VmGenIdState *vms; >>> + Object *obj = find_vmgenid_dev(); >>> + >>> + if (!obj) { >>> + return NULL; >>> + } >>> + vms = VMGENID(obj); >>> + >>> + info = g_malloc0(sizeof(*info)); >>> + info->guid = qemu_uuid_unparse_strdup(&vms->guid); >>> + return info; >>> +} >> >> This either returns a GuidInfo or NULL. >> >> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will >> crash like this: >> >> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6 >> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6 >> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6 >> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6 >> #4 0x0000555555c800a0 in visit_start_struct (v= >> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49 >> #5 0x0000555555c5d398 in visit_type_GuidInfo (v= >> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038 >> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148 >> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186 >> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97 >> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200) >> at /work/armbru/qemu/qapi/qmp-dispatch.c:124 >> #10 0x00005555557bb671 in handle_qmp_command (parser= >> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789 >> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1) >> at /work/armbru/qemu/qobject/json-streamer.c:105 >> >> Sorry for not having reviewed this earlier. On the other hand, how come >> this survived testing? > > We primarily focused on testing the functionality, not the failure / > corner cases, I think. (There are at least two other known startup > scenarios that are not handled correctly just yet, although they don't > crash -- multiple vmgenid devices, and vmgenid on machtypes without > writeable fw_cfg.) > > Also, in my testing I only called the monitor command via HMP (from > virsh), and AFAICS that one doesn't crash even if the device is missing. > Nonetheless, qmp_query_vm_generation_id() should set an error when it > returns NULL, yes. > > I think Ben wants to post a followup series with those fixes mentioned > above, in time for 2.9, perhaps this crash can be addressed in there > too. Ben? Regarding your other email ("New QMP command without a test -> automatic NAK"), Ben did write a small test suite for this feature: [Qemu-devel] [PATCH v8 7/8] tests: Add unit tests for the VM Generation ID feature and it contains a test called "/vmgenid/vmgenid/query-monitor". That patch is not part of the pull request because the functional tests in the same suite only pass if you have an updated SeaBIOS blob in the tree as well. While the SeaBIOS patches have been committed, the update for the blob bundled with QEMU is still in progress. Thus the patch with the tests is being delayed. So, it wasn't negligence, we simply missed this failure case because we were so focused on the long-awaited functionality. We didn't miss other failure cases that were a bit closer to the functionality. BTW, now that I look at said "/vmgenid/vmgenid/query-monitor" test case, it also doesn't seem to catch this error -- the monitor command is apparently not called without the device present. So yeah, review focused specifically on QMP / QAPI bits is always welcome. Thanks Laszlo
Laszlo Ersek <lersek@redhat.com> writes: > On 03/02/17 09:11, Markus Armbruster wrote: >> Crash bug, I'm afraid... >> >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >>> From: Igor Mammedov <imammedo@redhat.com> >>> >>> Add commands to query Virtual Machine Generation ID counter. >>> >>> QMP command example: >>> { "execute": "query-vm-generation-id" } >>> >>> HMP command example: >>> info vm-generation-id >>> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> Signed-off-by: Ben Warren <ben@skyportsystems.com> >>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>> Tested-by: Laszlo Ersek <lersek@redhat.com> >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> qapi-schema.json | 20 ++++++++++++++++++++ >>> hmp.h | 1 + >>> hmp.c | 9 +++++++++ >>> hw/acpi/vmgenid.c | 16 ++++++++++++++++ >>> stubs/vmgenid.c | 9 +++++++++ >>> hmp-commands-info.hx | 14 ++++++++++++++ >>> stubs/Makefile.objs | 1 + >>> 7 files changed, 70 insertions(+) >>> create mode 100644 stubs/vmgenid.c >>> >>> diff --git a/qapi-schema.json b/qapi-schema.json >>> index d6186d4..84692da 100644 >>> --- a/qapi-schema.json >>> +++ b/qapi-schema.json >>> @@ -6188,3 +6188,23 @@ >>> # >>> ## >>> { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] } >>> + >>> +## >>> +# @GuidInfo: >>> +# >>> +# GUID information. >>> +# >>> +# @guid: the globally unique identifier >>> +# >>> +# Since: 2.9 >>> +## >>> +{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} } >>> + >>> +## >>> +# @query-vm-generation-id: >>> +# >>> +# Show Virtual Machine Generation ID >>> +# >>> +# Since 2.9 >>> +## >>> +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } >> [...] >>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c >>> index c8465df..744f284 100644 >>> --- a/hw/acpi/vmgenid.c >>> +++ b/hw/acpi/vmgenid.c >>> @@ -240,3 +240,19 @@ static void vmgenid_register_types(void) >>> } >>> >>> type_init(vmgenid_register_types) >>> + >>> +GuidInfo *qmp_query_vm_generation_id(Error **errp) >>> +{ >>> + GuidInfo *info; >>> + VmGenIdState *vms; >>> + Object *obj = find_vmgenid_dev(); >>> + >>> + if (!obj) { >>> + return NULL; >>> + } >>> + vms = VMGENID(obj); >>> + >>> + info = g_malloc0(sizeof(*info)); >>> + info->guid = qemu_uuid_unparse_strdup(&vms->guid); >>> + return info; >>> +} >> >> This either returns a GuidInfo or NULL. >> >> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will >> crash like this: >> >> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6 >> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6 >> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6 >> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6 >> #4 0x0000555555c800a0 in visit_start_struct (v= >> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49 >> #5 0x0000555555c5d398 in visit_type_GuidInfo (v= >> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038 >> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148 >> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186 >> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97 >> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200) >> at /work/armbru/qemu/qapi/qmp-dispatch.c:124 >> #10 0x00005555557bb671 in handle_qmp_command (parser= >> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789 >> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1) >> at /work/armbru/qemu/qobject/json-streamer.c:105 >> >> Sorry for not having reviewed this earlier. On the other hand, how come >> this survived testing? > > We primarily focused on testing the functionality, not the failure / > corner cases, I think. That's as natural as it is wrong :) I'm a lackluster tester myself. The only way I can bring myself to test systematically is write automated tests. Fortunately, our infrastructure for that is much better than it used to be. Our habits still seem to be trailing the infrastructure, though. See also "New QMP command without a test -> automatic NAK", Message-ID: <871sugkrw5.fsf@dusky.pond.sub.org>. > (There are at least two other known startup > scenarios that are not handled correctly just yet, although they don't > crash -- multiple vmgenid devices, and vmgenid on machtypes without > writeable fw_cfg.) > > Also, in my testing I only called the monitor command via HMP (from > virsh), and AFAICS that one doesn't crash even if the device is missing. Correct. > Nonetheless, qmp_query_vm_generation_id() should set an error when it > returns NULL, yes. That's the obvious fix. It's a one-liner. > I think Ben wants to post a followup series with those fixes mentioned > above, in time for 2.9, perhaps this crash can be addressed in there > too. Ben? Since the fix is a one-liner, I'd like you guys to consider respinning this pull request with the fix.
Laszlo Ersek <lersek@redhat.com> writes: > On 03/02/17 09:25, Laszlo Ersek wrote: > > Regarding your other email ("New QMP command without a test -> automatic > NAK"), Ben did write a small test suite for this feature: > > [Qemu-devel] [PATCH v8 7/8] tests: Add unit tests for the VM Generation > ID feature > > and it contains a test called "/vmgenid/vmgenid/query-monitor". > > That patch is not part of the pull request because the functional tests > in the same suite only pass if you have an updated SeaBIOS blob in the > tree as well. While the SeaBIOS patches have been committed, the update > for the blob bundled with QEMU is still in progress. Thus the patch with > the tests is being delayed. Posting the testing part that depends on in-progress work only as RFC sounds like a perfectly acceptable case of the "not practical" exception mentioned "New QMP command without a test -> automatic NAK". > So, it wasn't negligence, we simply missed this failure case because we > were so focused on the long-awaited functionality. We didn't miss other > failure cases that were a bit closer to the functionality. > > BTW, now that I look at said "/vmgenid/vmgenid/query-monitor" test case, > it also doesn't seem to catch this error -- the monitor command is > apparently not called without the device present. I expect you to try to cover all QMP command error cases with automated tests. I don't expect you to always succeed. We're all human :) > So yeah, review > focused specifically on QMP / QAPI bits is always welcome. It's a struggle, to be honest. Bits of QMP and QAPI are often buried deep in patches dealing with other stuff. QMP can usually be separated into its own patch, but QAPI is just infrastructure, it *wants* to be used.
On 03/02/17 09:42, Markus Armbruster wrote: > Laszlo Ersek <lersek@redhat.com> writes: > >> On 03/02/17 09:11, Markus Armbruster wrote: >>> Crash bug, I'm afraid... >>> >>> "Michael S. Tsirkin" <mst@redhat.com> writes: >>> >>>> From: Igor Mammedov <imammedo@redhat.com> >>>> >>>> Add commands to query Virtual Machine Generation ID counter. >>>> >>>> QMP command example: >>>> { "execute": "query-vm-generation-id" } >>>> >>>> HMP command example: >>>> info vm-generation-id >>>> >>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>>> Signed-off-by: Ben Warren <ben@skyportsystems.com> >>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>>> Tested-by: Laszlo Ersek <lersek@redhat.com> >>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>> --- >>>> qapi-schema.json | 20 ++++++++++++++++++++ >>>> hmp.h | 1 + >>>> hmp.c | 9 +++++++++ >>>> hw/acpi/vmgenid.c | 16 ++++++++++++++++ >>>> stubs/vmgenid.c | 9 +++++++++ >>>> hmp-commands-info.hx | 14 ++++++++++++++ >>>> stubs/Makefile.objs | 1 + >>>> 7 files changed, 70 insertions(+) >>>> create mode 100644 stubs/vmgenid.c >>>> >>>> diff --git a/qapi-schema.json b/qapi-schema.json >>>> index d6186d4..84692da 100644 >>>> --- a/qapi-schema.json >>>> +++ b/qapi-schema.json >>>> @@ -6188,3 +6188,23 @@ >>>> # >>>> ## >>>> { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] } >>>> + >>>> +## >>>> +# @GuidInfo: >>>> +# >>>> +# GUID information. >>>> +# >>>> +# @guid: the globally unique identifier >>>> +# >>>> +# Since: 2.9 >>>> +## >>>> +{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} } >>>> + >>>> +## >>>> +# @query-vm-generation-id: >>>> +# >>>> +# Show Virtual Machine Generation ID >>>> +# >>>> +# Since 2.9 >>>> +## >>>> +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } >>> [...] >>>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c >>>> index c8465df..744f284 100644 >>>> --- a/hw/acpi/vmgenid.c >>>> +++ b/hw/acpi/vmgenid.c >>>> @@ -240,3 +240,19 @@ static void vmgenid_register_types(void) >>>> } >>>> >>>> type_init(vmgenid_register_types) >>>> + >>>> +GuidInfo *qmp_query_vm_generation_id(Error **errp) >>>> +{ >>>> + GuidInfo *info; >>>> + VmGenIdState *vms; >>>> + Object *obj = find_vmgenid_dev(); >>>> + >>>> + if (!obj) { >>>> + return NULL; >>>> + } >>>> + vms = VMGENID(obj); >>>> + >>>> + info = g_malloc0(sizeof(*info)); >>>> + info->guid = qemu_uuid_unparse_strdup(&vms->guid); >>>> + return info; >>>> +} >>> >>> This either returns a GuidInfo or NULL. >>> >>> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will >>> crash like this: >>> >>> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6 >>> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6 >>> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6 >>> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6 >>> #4 0x0000555555c800a0 in visit_start_struct (v= >>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49 >>> #5 0x0000555555c5d398 in visit_type_GuidInfo (v= >>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038 >>> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148 >>> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186 >>> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97 >>> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200) >>> at /work/armbru/qemu/qapi/qmp-dispatch.c:124 >>> #10 0x00005555557bb671 in handle_qmp_command (parser= >>> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789 >>> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1) >>> at /work/armbru/qemu/qobject/json-streamer.c:105 >>> >>> Sorry for not having reviewed this earlier. On the other hand, how come >>> this survived testing? >> >> We primarily focused on testing the functionality, not the failure / >> corner cases, I think. > > That's as natural as it is wrong :) > > I'm a lackluster tester myself. The only way I can bring myself to test > systematically is write automated tests. Fortunately, our > infrastructure for that is much better than it used to be. Our habits > still seem to be trailing the infrastructure, though. > > See also "New QMP command without a test -> automatic NAK", Message-ID: > <871sugkrw5.fsf@dusky.pond.sub.org>. > >> (There are at least two other known startup >> scenarios that are not handled correctly just yet, although they don't >> crash -- multiple vmgenid devices, and vmgenid on machtypes without >> writeable fw_cfg.) >> >> Also, in my testing I only called the monitor command via HMP (from >> virsh), and AFAICS that one doesn't crash even if the device is missing. > > Correct. > >> Nonetheless, qmp_query_vm_generation_id() should set an error when it >> returns NULL, yes. > > That's the obvious fix. It's a one-liner. > >> I think Ben wants to post a followup series with those fixes mentioned >> above, in time for 2.9, perhaps this crash can be addressed in there >> too. Ben? > > Since the fix is a one-liner, I'd like you guys to consider respinning > this pull request with the fix. If Ben & Michael send out a new pull with the above fixed, I'd like to point out that the updated SeaBIOS blob is now in the tree (see commit 8779fccbef0c, "seabios: update to 1.10.2 release", 2017-02-28), so the unit test patch can be included as well. Thanks Laszlo
I > On Mar 2, 2017, at 5:15 AM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 03/02/17 09:42, Markus Armbruster wrote: >> Laszlo Ersek <lersek@redhat.com> writes: >> >>> On 03/02/17 09:11, Markus Armbruster wrote: >>>> Crash bug, I'm afraid... >>>> >>>> "Michael S. Tsirkin" <mst@redhat.com> writes: >>>> >>>>> From: Igor Mammedov <imammedo@redhat.com> >>>>> >>>>> Add commands to query Virtual Machine Generation ID counter. >>>>> >>>>> QMP command example: >>>>> { "execute": "query-vm-generation-id" } >>>>> >>>>> HMP command example: >>>>> info vm-generation-id >>>>> >>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>>>> Signed-off-by: Ben Warren <ben@skyportsystems.com> >>>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>>>> Tested-by: Laszlo Ersek <lersek@redhat.com> >>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>> --- >>>>> qapi-schema.json | 20 ++++++++++++++++++++ >>>>> hmp.h | 1 + >>>>> hmp.c | 9 +++++++++ >>>>> hw/acpi/vmgenid.c | 16 ++++++++++++++++ >>>>> stubs/vmgenid.c | 9 +++++++++ >>>>> hmp-commands-info.hx | 14 ++++++++++++++ >>>>> stubs/Makefile.objs | 1 + >>>>> 7 files changed, 70 insertions(+) >>>>> create mode 100644 stubs/vmgenid.c >>>>> >>>>> diff --git a/qapi-schema.json b/qapi-schema.json >>>>> index d6186d4..84692da 100644 >>>>> --- a/qapi-schema.json >>>>> +++ b/qapi-schema.json >>>>> @@ -6188,3 +6188,23 @@ >>>>> # >>>>> ## >>>>> { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] } >>>>> + >>>>> +## >>>>> +# @GuidInfo: >>>>> +# >>>>> +# GUID information. >>>>> +# >>>>> +# @guid: the globally unique identifier >>>>> +# >>>>> +# Since: 2.9 >>>>> +## >>>>> +{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} } >>>>> + >>>>> +## >>>>> +# @query-vm-generation-id: >>>>> +# >>>>> +# Show Virtual Machine Generation ID >>>>> +# >>>>> +# Since 2.9 >>>>> +## >>>>> +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } >>>> [...] >>>>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c >>>>> index c8465df..744f284 100644 >>>>> --- a/hw/acpi/vmgenid.c >>>>> +++ b/hw/acpi/vmgenid.c >>>>> @@ -240,3 +240,19 @@ static void vmgenid_register_types(void) >>>>> } >>>>> >>>>> type_init(vmgenid_register_types) >>>>> + >>>>> +GuidInfo *qmp_query_vm_generation_id(Error **errp) >>>>> +{ >>>>> + GuidInfo *info; >>>>> + VmGenIdState *vms; >>>>> + Object *obj = find_vmgenid_dev(); >>>>> + >>>>> + if (!obj) { >>>>> + return NULL; >>>>> + } >>>>> + vms = VMGENID(obj); >>>>> + >>>>> + info = g_malloc0(sizeof(*info)); >>>>> + info->guid = qemu_uuid_unparse_strdup(&vms->guid); >>>>> + return info; >>>>> +} >>>> >>>> This either returns a GuidInfo or NULL. >>>> >>>> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will >>>> crash like this: >>>> >>>> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6 >>>> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6 >>>> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6 >>>> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6 >>>> #4 0x0000555555c800a0 in visit_start_struct (v= >>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49 >>>> #5 0x0000555555c5d398 in visit_type_GuidInfo (v= >>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038 >>>> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148 >>>> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186 >>>> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97 >>>> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200) >>>> at /work/armbru/qemu/qapi/qmp-dispatch.c:124 >>>> #10 0x00005555557bb671 in handle_qmp_command (parser= >>>> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789 >>>> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1) >>>> at /work/armbru/qemu/qobject/json-streamer.c:105 >>>> >>>> Sorry for not having reviewed this earlier. On the other hand, how come >>>> this survived testing? >>> >>> We primarily focused on testing the functionality, not the failure / >>> corner cases, I think. >> >> That's as natural as it is wrong :) >> >> I'm a lackluster tester myself. The only way I can bring myself to test >> systematically is write automated tests. Fortunately, our >> infrastructure for that is much better than it used to be. Our habits >> still seem to be trailing the infrastructure, though. >> >> See also "New QMP command without a test -> automatic NAK", Message-ID: >> <871sugkrw5.fsf@dusky.pond.sub.org>. >> >>> (There are at least two other known startup >>> scenarios that are not handled correctly just yet, although they don't >>> crash -- multiple vmgenid devices, and vmgenid on machtypes without >>> writeable fw_cfg.) >>> >>> Also, in my testing I only called the monitor command via HMP (from >>> virsh), and AFAICS that one doesn't crash even if the device is missing. >> >> Correct. >> >>> Nonetheless, qmp_query_vm_generation_id() should set an error when it >>> returns NULL, yes. >> >> That's the obvious fix. It's a one-liner. >> >>> I think Ben wants to post a followup series with those fixes mentioned >>> above, in time for 2.9, perhaps this crash can be addressed in there >>> too. Ben? >> >> Since the fix is a one-liner, I'd like you guys to consider respinning >> this pull request with the fix. > > If Ben & Michael send out a new pull with the above fixed, I'd like to > point out that the updated SeaBIOS blob is now in the tree (see commit > 8779fccbef0c, "seabios: update to 1.10.2 release", 2017-02-28), so the > unit test patch can be included as well. > Yes, the SeaBIOS image is now pulled in, so it’d be great to pull in the unit tests too. What do I do here - send a patch with the fix, or a re-spin of the single original patch without the bug, or a re-spin of the patch series? > Thanks > Laszlo > > —Ben
On Thu, Mar 02, 2017 at 06:50:45AM -0800, Ben Warren wrote: > I > > On Mar 2, 2017, at 5:15 AM, Laszlo Ersek <lersek@redhat.com> wrote: > > > > On 03/02/17 09:42, Markus Armbruster wrote: > >> Laszlo Ersek <lersek@redhat.com> writes: > >> > >>> On 03/02/17 09:11, Markus Armbruster wrote: > >>>> Crash bug, I'm afraid... > >>>> > >>>> "Michael S. Tsirkin" <mst@redhat.com> writes: > >>>> > >>>>> From: Igor Mammedov <imammedo@redhat.com> > >>>>> > >>>>> Add commands to query Virtual Machine Generation ID counter. > >>>>> > >>>>> QMP command example: > >>>>> { "execute": "query-vm-generation-id" } > >>>>> > >>>>> HMP command example: > >>>>> info vm-generation-id > >>>>> > >>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> > >>>>> Reviewed-by: Eric Blake <eblake@redhat.com> > >>>>> Signed-off-by: Ben Warren <ben@skyportsystems.com> > >>>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> > >>>>> Tested-by: Laszlo Ersek <lersek@redhat.com> > >>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>>>> --- > >>>>> qapi-schema.json | 20 ++++++++++++++++++++ > >>>>> hmp.h | 1 + > >>>>> hmp.c | 9 +++++++++ > >>>>> hw/acpi/vmgenid.c | 16 ++++++++++++++++ > >>>>> stubs/vmgenid.c | 9 +++++++++ > >>>>> hmp-commands-info.hx | 14 ++++++++++++++ > >>>>> stubs/Makefile.objs | 1 + > >>>>> 7 files changed, 70 insertions(+) > >>>>> create mode 100644 stubs/vmgenid.c > >>>>> > >>>>> diff --git a/qapi-schema.json b/qapi-schema.json > >>>>> index d6186d4..84692da 100644 > >>>>> --- a/qapi-schema.json > >>>>> +++ b/qapi-schema.json > >>>>> @@ -6188,3 +6188,23 @@ > >>>>> # > >>>>> ## > >>>>> { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] } > >>>>> + > >>>>> +## > >>>>> +# @GuidInfo: > >>>>> +# > >>>>> +# GUID information. > >>>>> +# > >>>>> +# @guid: the globally unique identifier > >>>>> +# > >>>>> +# Since: 2.9 > >>>>> +## > >>>>> +{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} } > >>>>> + > >>>>> +## > >>>>> +# @query-vm-generation-id: > >>>>> +# > >>>>> +# Show Virtual Machine Generation ID > >>>>> +# > >>>>> +# Since 2.9 > >>>>> +## > >>>>> +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } > >>>> [...] > >>>>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > >>>>> index c8465df..744f284 100644 > >>>>> --- a/hw/acpi/vmgenid.c > >>>>> +++ b/hw/acpi/vmgenid.c > >>>>> @@ -240,3 +240,19 @@ static void vmgenid_register_types(void) > >>>>> } > >>>>> > >>>>> type_init(vmgenid_register_types) > >>>>> + > >>>>> +GuidInfo *qmp_query_vm_generation_id(Error **errp) > >>>>> +{ > >>>>> + GuidInfo *info; > >>>>> + VmGenIdState *vms; > >>>>> + Object *obj = find_vmgenid_dev(); > >>>>> + > >>>>> + if (!obj) { > >>>>> + return NULL; > >>>>> + } > >>>>> + vms = VMGENID(obj); > >>>>> + > >>>>> + info = g_malloc0(sizeof(*info)); > >>>>> + info->guid = qemu_uuid_unparse_strdup(&vms->guid); > >>>>> + return info; > >>>>> +} > >>>> > >>>> This either returns a GuidInfo or NULL. > >>>> > >>>> If it returns NULL, the generated qmp_marshal_output_GuidInfo() will > >>>> crash like this: > >>>> > >>>> #0 0x00007fffdb4c7765 in raise () at /lib64/libc.so.6 > >>>> #1 0x00007fffdb4c936a in abort () at /lib64/libc.so.6 > >>>> #2 0x00007fffdb4bff97 in __assert_fail_base () at /lib64/libc.so.6 > >>>> #3 0x00007fffdb4c0042 in () at /lib64/libc.so.6 > >>>> #4 0x0000555555c800a0 in visit_start_struct (v= > >>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, size=8, errp=0x7fffffffc790) at /work/armbru/qemu/qapi/qapi-visit-core.c:49 > >>>> #5 0x0000555555c5d398 in visit_type_GuidInfo (v= > >>>> 0x555557727000, name=0x555555cf29e8 "unused", obj=0x7fffffffc7c8, errp=0x7fffffffc7d8) at qapi-visit.c:6038 > >>>> #6 0x0000555555922fc0 in qmp_marshal_output_GuidInfo (ret_in=0x0, ret_out=0x7fffffffc870, errp=0x7fffffffc820) at qmp-marshal.c:5148 > >>>> #7 0x0000555555923123 in qmp_marshal_query_vm_generation_id (args=0x555557a67500, ret=0x7fffffffc870, errp=0x7fffffffc868) at qmp-marshal.c:5186 > >>>> #8 0x0000555555c83a73 in do_qmp_dispatch (request=0x555556ab3200, errp=0x7fffffffc8c0) at /work/armbru/qemu/qapi/qmp-dispatch.c:97 > >>>> #9 0x0000555555c83ba3 in qmp_dispatch (request=0x555556ab3200) > >>>> at /work/armbru/qemu/qapi/qmp-dispatch.c:124 > >>>> #10 0x00005555557bb671 in handle_qmp_command (parser= > >>>> 0x5555568776b0, tokens=0x555556858520) at /work/armbru/qemu/monitor.c:3789 > >>>> #11 0x0000555555c8af2f in json_message_process_token (lexer=0x5555568776b8, input=0x5555568582e0, type=JSON_RCURLY, x=48, y=1) > >>>> at /work/armbru/qemu/qobject/json-streamer.c:105 > >>>> > >>>> Sorry for not having reviewed this earlier. On the other hand, how come > >>>> this survived testing? > >>> > >>> We primarily focused on testing the functionality, not the failure / > >>> corner cases, I think. > >> > >> That's as natural as it is wrong :) > >> > >> I'm a lackluster tester myself. The only way I can bring myself to test > >> systematically is write automated tests. Fortunately, our > >> infrastructure for that is much better than it used to be. Our habits > >> still seem to be trailing the infrastructure, though. > >> > >> See also "New QMP command without a test -> automatic NAK", Message-ID: > >> <871sugkrw5.fsf@dusky.pond.sub.org>. > >> > >>> (There are at least two other known startup > >>> scenarios that are not handled correctly just yet, although they don't > >>> crash -- multiple vmgenid devices, and vmgenid on machtypes without > >>> writeable fw_cfg.) > >>> > >>> Also, in my testing I only called the monitor command via HMP (from > >>> virsh), and AFAICS that one doesn't crash even if the device is missing. > >> > >> Correct. > >> > >>> Nonetheless, qmp_query_vm_generation_id() should set an error when it > >>> returns NULL, yes. > >> > >> That's the obvious fix. It's a one-liner. > >> > >>> I think Ben wants to post a followup series with those fixes mentioned > >>> above, in time for 2.9, perhaps this crash can be addressed in there > >>> too. Ben? > >> > >> Since the fix is a one-liner, I'd like you guys to consider respinning > >> this pull request with the fix. > > > > If Ben & Michael send out a new pull with the above fixed, I'd like to > > point out that the updated SeaBIOS blob is now in the tree (see commit > > 8779fccbef0c, "seabios: update to 1.10.2 release", 2017-02-28), so the > > unit test patch can be included as well. > > > Yes, the SeaBIOS image is now pulled in, so it’d be great to pull in the unit tests too. What do I do here - send a patch with the fix, or a re-spin of the single original patch without the bug, or a re-spin of the patch series? > > Thanks > > Laszlo > > > > > —Ben > Send the fix first of all pls. Can you send pointer to the test patches pls? I have some version on a branch but I'd rather not guess.
diff --git a/qapi-schema.json b/qapi-schema.json index d6186d4..84692da 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -6188,3 +6188,23 @@ # ## { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] } + +## +# @GuidInfo: +# +# GUID information. +# +# @guid: the globally unique identifier +# +# Since: 2.9 +## +{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} } + +## +# @query-vm-generation-id: +# +# Show Virtual Machine Generation ID +# +# Since 2.9 +## +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } diff --git a/hmp.h b/hmp.h index 05daf7c..799fd37 100644 --- a/hmp.h +++ b/hmp.h @@ -137,5 +137,6 @@ void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict); void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict); void hmp_info_dump(Monitor *mon, const QDict *qdict); void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict); +void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict); #endif diff --git a/hmp.c b/hmp.c index 83e287e..aadbcf5 100644 --- a/hmp.c +++ b/hmp.c @@ -2576,3 +2576,12 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict) qapi_free_HotpluggableCPUList(saved); } + +void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict) +{ + GuidInfo *info = qmp_query_vm_generation_id(NULL); + if (info) { + monitor_printf(mon, "%s\n", info->guid); + } + qapi_free_GuidInfo(info); +} diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c index c8465df..744f284 100644 --- a/hw/acpi/vmgenid.c +++ b/hw/acpi/vmgenid.c @@ -240,3 +240,19 @@ static void vmgenid_register_types(void) } type_init(vmgenid_register_types) + +GuidInfo *qmp_query_vm_generation_id(Error **errp) +{ + GuidInfo *info; + VmGenIdState *vms; + Object *obj = find_vmgenid_dev(); + + if (!obj) { + return NULL; + } + vms = VMGENID(obj); + + info = g_malloc0(sizeof(*info)); + info->guid = qemu_uuid_unparse_strdup(&vms->guid); + return info; +} diff --git a/stubs/vmgenid.c b/stubs/vmgenid.c new file mode 100644 index 0000000..c64eb7a --- /dev/null +++ b/stubs/vmgenid.c @@ -0,0 +1,9 @@ +#include "qemu/osdep.h" +#include "qmp-commands.h" +#include "qapi/qmp/qerror.h" + +GuidInfo *qmp_query_vm_generation_id(Error **errp) +{ + error_setg(errp, QERR_UNSUPPORTED); + return NULL; +} diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx index b0f35e6..a53f105 100644 --- a/hmp-commands-info.hx +++ b/hmp-commands-info.hx @@ -802,6 +802,20 @@ Show information about hotpluggable CPUs ETEXI STEXI +@item info vm-generation-id +@findex vm-generation-id +Show Virtual Machine Generation ID +ETEXI + + { + .name = "vm-generation-id", + .args_type = "", + .params = "", + .help = "Show Virtual Machine Generation ID", + .cmd = hmp_info_vm_generation_id, + }, + +STEXI @end table ETEXI diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index aa6050f..224f04b 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -36,3 +36,4 @@ stub-obj-y += qmp_pc_dimm_device_list.o stub-obj-y += target-monitor-defs.o stub-obj-y += target-get-monitor-def.o stub-obj-y += pc_madt_cpu_entry.o +stub-obj-y += vmgenid.o