Re: [Qemu-devel] [PATCH 12/17] rtc: add a QOM property for accessing device state

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Tue, Jun 05, 2012 at 04:14:44PM +0200, Paolo Bonzini wrote:
> Il 05/06/2012 03:00, Michael Roth ha scritto:
> > This will eventually be used to serialize device state for the purposes
> > of migration/savevm, and is also useful for introspection/testing.
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  hw/mc146818rtc.c |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> > index 7490d28..2dfc233 100644
> > --- a/hw/mc146818rtc.c
> > +++ b/hw/mc146818rtc.c
> > @@ -26,6 +26,7 @@
> >  #include "sysemu.h"
> >  #include "mc146818rtc.h"
> >  #include "mc146818rtc_state.h"
> > +#include "qapi-generated/mc146818rtc-qapi-visit.h"
> >  
> >  #ifdef TARGET_I386
> >  #include "apic.h"
> > @@ -590,6 +591,14 @@ static void rtc_get_date(Object *obj, Visitor *v, void *opaque,
> >      visit_end_struct(v, errp);
> >  }
> >  
> > +static void rtc_get_state(Object *obj, Visitor *v, void *opaque,
> > +                         const char *name, Error **errp)
> > +{
> > +    ISADevice *isa = ISA_DEVICE(obj);
> > +    RTCState *s = DO_UPCAST(RTCState, dev, isa);
> > +    visit_type_RTCState(v, &s, name, errp);
> > +}
> > +
> >  static int rtc_initfn(ISADevice *dev)
> >  {
> >      RTCState *s = DO_UPCAST(RTCState, dev, dev);
> > @@ -638,6 +647,9 @@ static int rtc_initfn(ISADevice *dev)
> >      object_property_add(OBJECT(s), "date", "struct tm",
> >                          rtc_get_date, NULL, NULL, s, NULL);
> >  
> > +    object_property_add(OBJECT(s), "state", "RTCState",
> > +                        rtc_get_state, NULL, NULL, s, NULL);
> > +
> >      return 0;
> >  }
> >  
> 
> Nice!  The next obvious step would be to use this from vmstate instead
> of hardcoded struct offsets.

Heh, indeed. I actually looked into this quite a bit thinking it was the
next logical progression and was hoping to make this part of the RFC.

The main issues I ran into were the numerous cases where vmstate
descriptions will "flatten" embedded struct fields rather than marking them as
VMS_STRUCT, as it creates a hard incompatibility with, for instance, the
way "struct tm current_tm" is serialized. We end up needing to do things
like parsing VMStateField.name for "." characters so we know additional
levels of traversal are needed to fetch the serialized data. There are also
areas where we unroll arrays that cause similar pains.

I think it's still doable, but it ends up adding a lot of complexity to
the last place we need it.

The main benefit, in my mind, was decoupling vmstate from the actual device
state such that the vmstate field descriptions became purely a stable "wire"
protocol of sorts, and the serialized data could be transformed to
maintain that compatibility regardless of how much an actual device's
structure changed. But the cases where manipulating a QObject would
cover more cases than adding/modifying vmstate field descriptors seem
like they'd be too rare to warrant the added complexity. Having this
capability in the context of a wire protocol that's better seperated from the
data representation makes sense, but in the case of vmstate there's too much
overlap and too much complexity IMO.

> 
> Paolo
>