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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Amit Shah (amit.shah@redhat.com) wrote:
> On (Wed) 21 May 2014 [10:44:07], Dr. David Alan Gilbert wrote:
> > * Amit Shah (amit.shah@redhat.com) 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 <amit.shah@redhat.com>
> > > ---
> > >  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
> > > +@item -dump-vmstate @var{file}
> > > +@findex -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 / dgilbert@redhat.com / Manchester, UK