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

[Ben Warren]

I
> On Mar 2, 2017, at 5:15 AM, Laszlo Ersek <address@hidden> wrote:
> 
> On 03/02/17 09:42, Markus Armbruster wrote:
>> 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.
> 
> 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

smime.p7s
Description: S/MIME cryptographic signature