* Halil Pasic (pa...@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 / dgilb...@redhat.com / Manchester, UK
