Re: [Qemu-devel] [RFC PATCH 3/4] migration/vmstate: fix array of pointers to struct

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

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 10/25/2016 12:13 PM, Dr. David Alan Gilbert wrote:
> [..]
> >>              for (i = 0; i < n_elems; i++) {
> >> -                void *addr = base_addr + size * i;
> >> +                void *curr_elem = first_elem + size * i;
> > 
> > This diff is quite confusing because a lot of it involves the
> > rename of 'addr' to 'curr_elem' at the same time as you change
> > the structure.  It would be better to split the renaming into
> > a separate patch to make this clearer or just leave the name
> > the same.
> > 
> 
> You are absolutely right this is a Frankestein of a cleanup
> patch and the actual patch. I will split the cleanup out.
> 
> The patch is also conceptually based on my remove .start patch
> it's just that I wanted to make the RFC independent so it can
> be tested more easily.
> 
> [..]
> >> @@ -720,6 +722,27 @@ 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);
> >> +    assert(tmp == 0);
> > 
> > There's no need for the assert there, just return -EINVAL,
> > then we'll get a clear error.
> 
> Will do.
> 
> > Also, '0' is a bad value to use just as a check - if the field is wrong then
> > 0 often appears in the next byte anyway; 
> > 
> 
> Absolutely right. How about -1?

-1 is OK (although you could use any character - e.g. N (for Null)).

> > However, I'm not sure it's worth having the info_nullptr;
> > if we just leave out the whole info_nullptr then you should still
> > be protected by the section footer, although this may be
> > able to give a better error.
> > 
> 
> IMHO this can (in some cases) guard against the case we have the
> same number of elements on source and on target, but at different
> positions (e.g. {ptr0, NULL, NULL} and {NULL, ptr1, NULL}. The footers
> should not be able to detect this.

Yes, you're right it does give that extra protection and is worth it.

Dave

> Thank you very much for the thorough review! I will wait a bit
> to see if more discussion happens, and then send out a non RFC
> version with the issues addressed.
> 
> Halil
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK