[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 01/18] migration: dump vmstate info as a json fi
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH 01/18] migration: dump vmstate info as a json file for static analysis |
Date: |
Wed, 21 May 2014 11:03:04 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
* Amit Shah (address@hidden) wrote:
> On (Wed) 21 May 2014 [10:44:07], Dr. David Alan Gilbert wrote:
> > * Amit Shah (address@hidden) wrote:
> > > This commit adds a new command, '-dump-vmstate', that takes a filename
> > > as a parameter. When executed, QEMU will dump the vmstate information
> > > for the machine type it's invoked with to the file, and quit.
> > >
> > > The JSON-format output can then be used to compare the vmstate info for
> > > different QEMU versions, specifically to test whether live migration
> > > would break due to changes in the vmstate data.
> > >
> > > This is based on a version from Andreas Färber posted here:
> > > https://lists.gnu.org/archive/html/qemu-devel/2013-10/msg03095.html
> > >
> > > A Python script that compares the output of such JSON dumps is included
> > > in the following commit.
> > >
> > > Signed-off-by: Amit Shah <address@hidden>
> > > ---
> > > include/migration/vmstate.h | 2 +
> > > qemu-options.hx | 9 +++
> > > savevm.c | 134
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > > vl.c | 14 +++++
> > > 4 files changed, 159 insertions(+)
> > >
> > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > > index 7e45048..9829c0e 100644
> > > --- a/include/migration/vmstate.h
> > > +++ b/include/migration/vmstate.h
> > > @@ -778,4 +778,6 @@ void vmstate_register_ram(struct MemoryRegion
> > > *memory, DeviceState *dev);
> > > void vmstate_unregister_ram(struct MemoryRegion *memory, DeviceState
> > > *dev);
> > > void vmstate_register_ram_global(struct MemoryRegion *memory);
> > >
> > > +void dump_vmstate_json_to_file(FILE *out_fp);
> > > +
> > > #endif
> > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > index 781af14..d376227 100644
> > > --- a/qemu-options.hx
> > > +++ b/qemu-options.hx
> > > @@ -3146,6 +3146,15 @@ STEXI
> > > prepend a timestamp to each log message.(default:on)
> > > ETEXI
> > >
> > > +DEF("dump-vmstate", HAS_ARG, QEMU_OPTION_dump_vmstate,
> > > + "-dump-vmstate <file>\n" "", QEMU_ARCH_ALL)
> > > +STEXI
> > > address@hidden -dump-vmstate @var{file}
> > > address@hidden -dump-vmstate
> > > +Dump json-encoded vmstate information for current machine type to file
> > > +in @var{file}
> > > +ETEXI
> > > +
> > > HXCOMM This is the last statement. Insert new options before this line!
> > > STEXI
> > > @end table
> > > diff --git a/savevm.c b/savevm.c
> > > index da8aa24..a4ce279 100644
> > > --- a/savevm.c
> > > +++ b/savevm.c
> > > @@ -24,6 +24,7 @@
> > >
> > > #include "config-host.h"
> > > #include "qemu-common.h"
> > > +#include "hw/boards.h"
> > > #include "hw/hw.h"
> > > #include "hw/qdev.h"
> > > #include "net/net.h"
> > > @@ -241,6 +242,139 @@ static QTAILQ_HEAD(savevm_handlers, SaveStateEntry)
> > > savevm_handlers =
> > > QTAILQ_HEAD_INITIALIZER(savevm_handlers);
> > > static int global_section_id;
> > >
> > > +static void dump_vmstate_vmsd(FILE *out_file,
> > > + const VMStateDescription *vmsd, int indent,
> > > + bool is_subsection);
> > > +
> > > +static void dump_vmstate_vmsf(FILE *out_file, const VMStateField *field,
> > > + int indent)
> >
> > checkpatch points out that some tabs managed to get into that indent line.
>
> Will fix.
>
> > Generally I think this patch is OK and quite useful; two thoughts:
> > 1) I was surprised it dumped every object type, rather than just those
> > that are instantiated; I think the latter would be more use in some
> > circumstances, since there's a load of weird and wonderful objects
> > that exist and are very rarely used.
>
> The idea is to be able to take a qemu binary and compare with another
> binary; if only fields that are instantiated are used, various
> invocations will have to be tried to find devices that may have
> broken.
>
> An alternative way of checking only devices which have been added to
> the running machine can be done via a monitor command (or a parameter
> to the existing cmdline option). But I'm not sure if that'll be more
> useful than the current one.
Or perhaps a way to dump that info and mask your checker with it if wanted?
> > 2) 'fields_exists' is a weird naming to put in the json file - it's
> > a function pointer for determining if the field is going to be
> > present;
> > maybe renaming as 'conditional' would make sense.
>
> Yea; I don't know if field_exists is going to be useful anyway. It's
> runtime info rather than static, so perhaps can just be dropped.
> Right now, anyway, the checker doesn't make use of this field at all.
I think it's useful to have field_exists because it lets you know that it's
conditional, I just think it's weird naming it like that in the json, since
an entry in the json that says 'fields_exists: true' sounds like the field
always exists, which is the opposite of what it means. It's just a naming
thing here.
Dave
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
[Qemu-devel] [PATCH 02/18] vmstate-static-checker: script to validate vmstate changes, Amit Shah, 2014/05/12
[Qemu-devel] [PATCH 03/18] tests: vmstate static checker: add dump1 and dump2 files, Amit Shah, 2014/05/12
[Qemu-devel] [PATCH 04/18] tests: vmstate static checker: incompat machine types, Amit Shah, 2014/05/12
[Qemu-devel] [PATCH 05/18] tests: vmstate static checker: add version error in main section, Amit Shah, 2014/05/12
[Qemu-devel] [PATCH 06/18] tests: vmstate static checker: version mismatch inside a Description, Amit Shah, 2014/05/12
[Qemu-devel] [PATCH 07/18] tests: vmstate static checker: minimum_version_id check, Amit Shah, 2014/05/12