[RFC,v2,3/5] vmstate: add VMS_MUST_EXIST

Can be used to verify a required field exists or validate
state in some other way.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/migration/vmstate.h |  1 +
 vmstate.c                   | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

* Michael S. Tsirkin (mst@redhat.com) wrote:
> Can be used to verify a required field exists or validate
> state in some other way.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  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?

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

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

On Mon, Mar 24, 2014 at 05:11:16PM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > Can be used to verify a required field exists or validate
> > state in some other way.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  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 / dgilbert@redhat.com / Manchester, UK

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Mon, Mar 24, 2014 at 05:11:16PM +0000, Dr. David Alan Gilbert wrote:

<snip>

> > 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?

Maybe; however most fields are always-present, unless they have a test
function or minimum version, so it's a little weird to add a 'required'
when that's the default.

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

Yep, that's fine - was just an idea.

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

On Tue, Mar 25, 2014 at 09:23:13AM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Mon, Mar 24, 2014 at 05:11:16PM +0000, Dr. David Alan Gilbert wrote:
> 
> <snip>
> 
> > > 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?
> 
> Maybe; however most fields are always-present, unless they have a test
> function or minimum version, so it's a little weird to add a 'required'
> when that's the default.

Right - here we say "there is a test function but it must return true".

I considered adding a separate callback but it worried me
that it's not clear how would it interact with the exist
flag or the version flag. Ideas?

> > 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.
> 
> Yep, that's fine - was just an idea.
> 
> Dave
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK