I > On Mar 2, 2017, at 5:15 AM, Laszlo Ersek wrote: > > On 03/02/17 09:42, Markus Armbruster wrote: >> Laszlo Ersek writes: >> >>> On 03/02/17 09:11, Markus Armbruster wrote: >>>> Crash bug, I'm afraid... >>>> >>>> "Michael S. Tsirkin" writes: >>>> >>>>> From: Igor Mammedov >>>>> >>>>> 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 >>>>> Reviewed-by: Eric Blake >>>>> Signed-off-by: Ben Warren >>>>> Reviewed-by: Laszlo Ersek >>>>> Tested-by: Laszlo Ersek >>>>> Reviewed-by: Michael S. Tsirkin >>>>> Signed-off-by: Michael S. Tsirkin >>>>> --- >>>>> 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