[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'i
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands |
Date: |
Thu, 2 Mar 2017 17:32:13 +0200 |
On Thu, Mar 02, 2017 at 06:50:45AM -0800, Ben Warren wrote:
> 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
>
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.
--
MST
- [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description, (continued)
- [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description, Michael S. Tsirkin, 2017/03/02
- [Qemu-devel] [PULL 03/15] ACPI: Add vmgenid blob storage to the build tables, Michael S. Tsirkin, 2017/03/02
- [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands, Michael S. Tsirkin, 2017/03/02
- Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands, Markus Armbruster, 2017/03/02
- Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands, Markus Armbruster, 2017/03/02
- Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands, Laszlo Ersek, 2017/03/02
- Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands, Ben Warren, 2017/03/02
- Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PULL 05/15] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands, Laszlo Ersek, 2017/03/02
[Qemu-devel] [PULL 04/15] ACPI: Add Virtual Machine Generation ID support, Michael S. Tsirkin, 2017/03/02
[Qemu-devel] [PULL 07/15] MAINTAINERS: Add VM Generation ID entries, Michael S. Tsirkin, 2017/03/02
[Qemu-devel] [PULL 06/15] tests: Move reusable ACPI code into a utility file, Michael S. Tsirkin, 2017/03/02
[Qemu-devel] [PULL 08/15] virtio: check for vring setup in virtio_queue_empty, Michael S. Tsirkin, 2017/03/02
[Qemu-devel] [PULL 10/15] virtio: invalidate memory in vring_set_avail_event(), Michael S. Tsirkin, 2017/03/02
[Qemu-devel] [PULL 09/15] virtio: guard vring access when setting notification, Michael S. Tsirkin, 2017/03/02
[Qemu-devel] [PULL 11/15] virtio: add missing region cache init in virtio_load(), Michael S. Tsirkin, 2017/03/02
[Qemu-devel] [PULL 12/15] virtio: unbreak virtio-pci with IOMMU after caching ring translations, Michael S. Tsirkin, 2017/03/02
[Qemu-devel] [PULL 14/15] tests/acpi: update DSDT after last patch, Michael S. Tsirkin, 2017/03/02