From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45358) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wn3b2-0002An-OS for qemu-devel@nongnu.org; Wed, 21 May 2014 06:18:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wn3ax-00018T-Uj for qemu-devel@nongnu.org; Wed, 21 May 2014 06:18:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20177) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wn3ax-00018L-LR for qemu-devel@nongnu.org; Wed, 21 May 2014 06:17:59 -0400 Date: Wed, 21 May 2014 15:25:02 +0530 From: Amit Shah Message-ID: <20140521095502.GA32726@grmbl.mre> References: <20140521094407.GC2589@work-vm> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140521094407.GC2589@work-vm> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 01/18] migration: dump vmstate info as a json file for static analysis List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: Juan Quintela , Markus Armbruster , Alexander Graf , qemu list , Paolo Bonzini , Andreas F?rber 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 filenam= e > > as a parameter. When executed, QEMU will dump the vmstate informatio= n > > for the machine type it's invoked with to the file, and quit. > >=20 > > The JSON-format output can then be used to compare the vmstate info f= or > > different QEMU versions, specifically to test whether live migration > > would break due to changes in the vmstate data. > >=20 > > This is based on a version from Andreas F=E4rber posted here: > > https://lists.gnu.org/archive/html/qemu-devel/2013-10/msg03095.html > >=20 > > A Python script that compares the output of such JSON dumps is includ= ed > > in the following commit. > >=20 > > Signed-off-by: Amit Shah > > --- > > include/migration/vmstate.h | 2 + > > qemu-options.hx | 9 +++ > > savevm.c | 134 ++++++++++++++++++++++++++++++++++= ++++++++++ > > vl.c | 14 +++++ > > 4 files changed, 159 insertions(+) > >=20 > > 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 *me= mory, DeviceState *dev); > > void vmstate_unregister_ram(struct MemoryRegion *memory, DeviceState= *dev); > > void vmstate_register_ram_global(struct MemoryRegion *memory); > > =20 > > +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 > > =20 > > +DEF("dump-vmstate", HAS_ARG, QEMU_OPTION_dump_vmstate, > > + "-dump-vmstate \n" "", QEMU_ARCH_ALL) > > +STEXI > > +@item -dump-vmstate @var{file} > > +@findex -dump-vmstate > > +Dump json-encoded vmstate information for current machine type to fi= le > > +in @var{file} > > +ETEXI > > + > > HXCOMM This is the last statement. Insert new options before this li= ne! > > 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 @@ > > =20 > > #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, SaveStateEn= try) savevm_handlers =3D > > QTAILQ_HEAD_INITIALIZER(savevm_handlers); > > static int global_section_id; > > =20 > > +static void dump_vmstate_vmsd(FILE *out_file, > > + const VMStateDescription *vmsd, int in= dent, > > + bool is_subsection); > > + > > +static void dump_vmstate_vmsf(FILE *out_file, const VMStateField *fi= eld, > > + int indent) >=20 > checkpatch points out that some tabs managed to get into that indent li= ne. 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 tho= se > that are instantiated; I think the latter would be more use in so= me > circumstances, since there's a load of weird and wonderful object= s > 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 pr= esent; > 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