Re: [Qemu-devel] [RFC v2 3/5] vmstate: add VMS_MUST_EXIST

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 05:11:16PM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (address@hidden) wrote:
> > Can be used to verify a required field exists or validate
> > state in some other way.
> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >  include/migration/vmstate.h |  1 +
> >  vmstate.c                   | 10 +++++++++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 3a1587e..eb90cef 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -101,6 +101,7 @@ enum VMStateFlags {
> >      VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size 
> > */
> >      VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
> >      VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
> > +    VMS_MUST_EXIST       = 0x1000, /* Field must exist in input */
> >  };
> >  
> >  typedef struct {
> > diff --git a/vmstate.c b/vmstate.c
> > index fe53735..4943b83 100644
> > --- a/vmstate.c
> > +++ b/vmstate.c
> > @@ -13,7 +13,7 @@ static int vmstate_n_elems(void *opaque, VMStateField 
> > *field)
> >  {
> >      int n_elems = 1;
> >  
> > -    if (!(field->flags & ~VMS_NONE)) {
> > +    if (!(field->flags & ~(VMS_NONE | VMS_MUST_EXIST))) {
> >          n_elems = 0;
> >      } else if (field->flags & VMS_ARRAY) {
> >          n_elems = field->num;
> > @@ -107,6 +107,9 @@ int vmstate_load_state(QEMUFile *f, const 
> > VMStateDescription *vmsd,
> >              }
> >          }
> >          field++;
> > +    } else if (field->flags & VMS_MUST_EXIST) {
> > +        fprintf(stderr, "Input validation failed: %s/%s\n", vmsd->name, 
> > field->name);
> > +        return -1;
> 
> I think your intent here is just to misuse the field_exist function pointer 
> as a call for a different reason as a hook for a validator; is it really worth
> misusing it like that or is something more explicit worth it?
> Perhaps something passed an Error** so it could pass back what was wrong?


Well adding a required field seems valuable by itself, does it not?

And there's no way to pass in Error** since none of the callers
has Error**: all of migration still uses stderr to pass
errors.

So we could add an API but it doesn't seem too valuable.

Since all callers will use this through a wrapper like VMSTATE_TEST,
it will be easy to change our mind later.


> >      }
> >      ret = vmstate_subsection_load(f, vmsd, opaque);
> >      if (ret != 0) {
> > @@ -148,6 +151,11 @@ void vmstate_save_state(QEMUFile *f, const 
> > VMStateDescription *vmsd,
> >                      field->info->put(f, addr, size);
> >                  }
> >              }
> > +        } else {
> > +            if (field->flags & VMS_MUST_EXIST) {
> > +                fprintf(stderr, "Input validation failed: %s/%s\n", 
> > vmsd->name, field->name);
> > +                assert(!(field->flags & VMS_MUST_EXIST));
> 
> Wrong message for the save side.


Thanks!

> Dave
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK