Re: [Qemu-devel] [PATCH 01/18] migration: dump vmstate info as a json file for static analysis

[Amit Shah]

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.

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

Thanks for taking a look!

                Amit