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


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.

   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.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK