Re: [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of pointers to struct

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 12/15/2016 01:33 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >> Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the reward
> >> for trying to migrate an array with some null pointers in it was an
> >> illegal memory access, that is a swift and painless death of the process.
> >> Let's make vmstate cope with this scenario at least for pointers to
> >> structs. The general approach is when we encounter a null pointer
> >> (element) instead of following the pointer to save/load the data behind
> >> it we save/load a placeholder. This way we can detect if we expected a
> >> null pointer at the load side but not null data was saved instead. Sadly
> >> all other error scenarios are not detected by this scheme (and would
> >> require the usage of the JSON meta data).
> >>
> >> Limitations: Does not work for pointers to primitives.
> > 
> > Please document that limitation in a comment in the vmstate.h near the
> > macros that use it.
> > 
> 
> After looking at this again I do not know what was my intention whit the
> limitations statement. It was probably about arrays of pointers to
> primitives, but then the statement is wrong. The same scheme does work
> for primitives too. It just that I had a previous version which had this
> limitation. 
> 
> It could also be about just pointer (not array of pointers), but the
> commit message does not mention this case at all. 
> 
> I will just drop the limitations statement.
> 
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> Reviewed-by:
> >> Guenther Hutzl <hutzl@linux.vnet.ibm.com> ---
> >>
> >> We will need this to load/save some on demand created state from within
> >> the channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for an
> >> example).
> >> ---
> >>  include/migration/vmstate.h |  5 +++++
> >>  migration/vmstate.c         | 32 ++++++++++++++++++++++++++++++--
> >>  2 files changed, 35 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >> index 5940db0..86d4aca 100644
> >> --- a/include/migration/vmstate.h
> >> +++ b/include/migration/vmstate.h
> >> @@ -236,6 +236,10 @@ extern const VMStateInfo vmstate_info_uint16;
> >>  extern const VMStateInfo vmstate_info_uint32;
> >>  extern const VMStateInfo vmstate_info_uint64;
> >>  
> >> +/** Put this in the stream when migrating a null pointer.*/
> >> +#define VMS_NULLPTR_MARKER ((int8_t) -1)
> > 
> > You seem to be making things harder by using a signed int everywhere
> > when you just want a byte;  I suggest using '0'
> > 
> 
> I'm fine with byte 0x30, but I have an irrational bad feeling about
> using a character literal here. I know, in practice should
> not matter, but the C11 standard says nothing about how members of the
> basic execution character set are represented, and it's also not among
> the 'usual QEMU platform assumptions' (AFAIK). Are you OK defining
> the constant/macro as Ox30U instead of '0'? 

0x30 is fine if you prefer (although I'd be pretty surprised if we worked
on a non-ASCII system!);  My only requirements are it's just a plain normal
unsigned char and it's not the type of value that's common (like 0x00).

> >> +extern const VMStateInfo vmstate_info_nullptr;
> >> +
> >>  extern const VMStateInfo vmstate_info_float64;
> >>  extern const VMStateInfo vmstate_info_cpudouble;
> >>  
> >> @@ -453,6 +457,7 @@ extern const VMStateInfo vmstate_info_bitmap;
> >>      .size       = sizeof(_type *),                                    \
> >>      .flags      = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER,         \
> >>      .offset     = vmstate_offset_array(_s, _f, _type*, _n),          \
> >> +    .info       = &vmstate_info_nullptr,                              \
> > 
> > What's this change for?
> 
> It's garbage. A remainder of a previous version I mentioned above. Will
> remove. Good catch!
> 
> > 
> >>  }
> >>  
> >>  #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index 10a7645..ce3490a 100644
> >> --- a/migration/vmstate.c
> >> +++ b/migration/vmstate.c
> >> @@ -109,7 +109,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
> >>                      curr_elem = *(void **)curr_elem;
> >>                  }
> >> -                if (field->flags & VMS_STRUCT) {
> >> +                if (!curr_elem) {
> >> +                    /* if null pointer check placeholder and do not follow */
> >> +                    assert(field->flags & VMS_ARRAY_OF_POINTER);
> >> +                    vmstate_info_nullptr.get(f, curr_elem, size);
> >> +                } else if (field->flags & VMS_STRUCT) {
> >>                      ret = vmstate_load_state(f, field->vmsd, curr_elem,
> >>                                               field->vmsd->version_id);
> >>                  } else {
> >> @@ -320,7 +324,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> >>                      assert(curr_elem);
> >>                      curr_elem = *(void **)curr_elem;
> >>                  }
> >> -                if (field->flags & VMS_STRUCT) {
> >> +                if (!curr_elem) {
> >> +                    /* if null pointer write placeholder and do not follow */
> >> +                    assert(field->flags & VMS_ARRAY_OF_POINTER);
> >> +                    vmstate_info_nullptr.put(f, curr_elem, size);
> >> +                } else if (field->flags & VMS_STRUCT) {
> >>                      vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
> >>                  } else {
> >>                      field->info->put(f, curr_elem, size);
> >> @@ -708,6 +716,26 @@ const VMStateInfo vmstate_info_uint64 = {
> >>      .put  = put_uint64,
> >>  };
> >>  
> >> +static int get_nullptr(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> +    int8_t tmp;
> >> +    qemu_get_s8s(f, &tmp);
> > 
> > Now that I've suggested using just a character you can turn that
> > into:
> > 
> >   return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> > 
> 
> You are right! I was over-complicating. Will do.
> 
> >> +    return tmp == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> >> +}
> >> +
> >> +static void put_nullptr(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> +    int8_t tmp = VMS_NULLPTR_MARKER;
> >> +    assert(pv == NULL);
> >> +    qemu_put_s8s(f, &tmp);
> > 
> > and similarly put_byte.
> > 
> 
> Will do.
> 
> > Dave
> > 
> 
> Thanks for the review. I have already adopted mostly your proposals,
> but I will wait a bit in the hope that the fate of this patch borrowed
> form my other patch set (subject:"[PATCH v2 2/2] migration: drop unused
> VMStateField.start" gets resolved.

Yep, I'll look at that in a minute.

Dave

> By the way you could answer my question regarding that yourself, since
> you became co-maintainer for migration starting 2017-01-24. Congrats :).
> 
> 
> Regards,
> Halil
> 
> >> +}
> >> +
> >> +const VMStateInfo vmstate_info_nullptr = {
> >> +    .name = "uint64",
> >> +    .get  = get_nullptr,
> >> +    .put  = put_nullptr,
> >> +};
> >> +
> >>  /* 64 bit unsigned int. See that the received value is the same than the one
> >>     in the field */
> >>  
> >> -- 
> >> 2.8.4
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK