Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

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

>                        (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.