[3/5] migration/vmstate: fix array of ptr with nullptrs

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.

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.

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 in the
(s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
an example).
---
 include/migration/vmstate.h |  4 ++++
 migration/vmstate.c         | 33 +++++++++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 2 deletions(-)

* 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.
> 
> 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.
> 
> 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 in the
> (s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
> an example).
> ---
>  include/migration/vmstate.h |  4 ++++
>  migration/vmstate.c         | 33 +++++++++++++++++++++++++++++++--
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 63e7b02..f2dbf84 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -253,6 +253,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 (0x30U) /* '0' */
> +extern const VMStateInfo vmstate_info_nullptr;
> +
>  extern const VMStateInfo vmstate_info_float64;
>  extern const VMStateInfo vmstate_info_cpudouble;
>  
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 836a7a4..cb81cef 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -117,7 +117,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);

That can return an error instead of asserting.

> +                    vmstate_info_nullptr.get(f, curr_elem, size, NULL);

You've ignored the return value of the get; that should be  ret = 

> +                } else if (field->flags & VMS_STRUCT) {
>                      ret = vmstate_load_state(f, field->vmsd, curr_elem,
>                                               field->vmsd->version_id);
>                  } else {
> @@ -332,7 +336,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, NULL, NULL);
> +                } else if (field->flags & VMS_STRUCT) {
>                      vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
>                  } else {
>                      field->info->put(f, curr_elem, size, field, vmdesc_loop);
> @@ -747,6 +755,27 @@ const VMStateInfo vmstate_info_uint64 = {
>      .put  = put_uint64,
>  };
>  
> +static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
> +
> +{
> +    return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> +}
> +
> +static int put_nullptr(QEMUFile *f, void *pv, size_t size,
> +                        VMStateField *field, QJSON *vmdesc)
> +
> +{
> +    assert(pv == NULL);
> +    qemu_put_byte(f, VMS_NULLPTR_MARKER);
> +    return 0;
> +}

Again that assert could just turn into a return -EINVAL

Dave

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

On 02/17/2017 08:42 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.
>>
>> 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.
>>
>> 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 in the
>> (s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
>> an example).
>> ---
>>  include/migration/vmstate.h |  4 ++++
>>  migration/vmstate.c         | 33 +++++++++++++++++++++++++++++++--
>>  2 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 63e7b02..f2dbf84 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -253,6 +253,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 (0x30U) /* '0' */
>> +extern const VMStateInfo vmstate_info_nullptr;
>> +
>>  extern const VMStateInfo vmstate_info_float64;
>>  extern const VMStateInfo vmstate_info_cpudouble;
>>  
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 836a7a4..cb81cef 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -117,7 +117,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);
> 
> That can return an error instead of asserting.
> 
>> +                    vmstate_info_nullptr.get(f, curr_elem, size, NULL);
> 
> You've ignored the return value of the get; that should be  ret = 
> 
>> +                } else if (field->flags & VMS_STRUCT) {
>>                      ret = vmstate_load_state(f, field->vmsd, curr_elem,
>>                                               field->vmsd->version_id);
>>                  } else {
>> @@ -332,7 +336,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, NULL, NULL);
>> +                } else if (field->flags & VMS_STRUCT) {
>>                      vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
>>                  } else {
>>                      field->info->put(f, curr_elem, size, field, vmdesc_loop);
>> @@ -747,6 +755,27 @@ const VMStateInfo vmstate_info_uint64 = {
>>      .put  = put_uint64,
>>  };
>>  
>> +static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
>> +
>> +{
>> +    return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
>> +}
>> +
>> +static int put_nullptr(QEMUFile *f, void *pv, size_t size,
>> +                        VMStateField *field, QJSON *vmdesc)
>> +
>> +{
>> +    assert(pv == NULL);
>> +    qemu_put_byte(f, VMS_NULLPTR_MARKER);
>> +    return 0;
>> +}
> 
> Again that assert could just turn into a return -EINVAL
> 
> Dave
> 

@Dave: I have accidentally answered with 'reply' instead of 'reply all'
yesterday. Sorry for the noise!

Thanks for the review! You are right, my code is messy.

Yet I'm not sure what you propose is the best way to clean it up. My
concern is, that we are loosing some info about what exactly went wrong
(and where), if returning -EINVAL is not combined with additional
logging/error reporting. 

After re-checking the asserts they all indicate programming errors and
there is not much (IMHO) client code can do to recover and the user
should never observe. Unfortunately, I lack knowledge about how is the
client code handling errors and if there is something we need to clean
up. I assumed asserting is OK because of the per-existing asserts.

You are perfectly right about ignoring the return value of
vmstate_info_nullptr.get and at least I will have to fix that.
IMHO we have three options to fix this code:
a) Make vmstate_info_nullptr.get assert too as we expect null pointer
and we got not null is pretty much an indicator that we can't proceed
sanely. Possibly add ret = ... for aesthetic reasons. Keep the asserts.
b) Follow your suggestions without anything on top of it.
c) Follow your suggestions and add error_report stating what exactly
went wrong.

My priority is to get this in, so I will do what you say, or send
option b) late on Wednesday if no guidance until then.

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
>

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 02/17/2017 08:42 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.
> >>
> >> 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.
> >>
> >> 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 in the
> >> (s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
> >> an example).
> >> ---
> >>  include/migration/vmstate.h |  4 ++++
> >>  migration/vmstate.c         | 33 +++++++++++++++++++++++++++++++--
> >>  2 files changed, 35 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >> index 63e7b02..f2dbf84 100644
> >> --- a/include/migration/vmstate.h
> >> +++ b/include/migration/vmstate.h
> >> @@ -253,6 +253,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 (0x30U) /* '0' */
> >> +extern const VMStateInfo vmstate_info_nullptr;
> >> +
> >>  extern const VMStateInfo vmstate_info_float64;
> >>  extern const VMStateInfo vmstate_info_cpudouble;
> >>  
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index 836a7a4..cb81cef 100644
> >> --- a/migration/vmstate.c
> >> +++ b/migration/vmstate.c
> >> @@ -117,7 +117,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);
> > 
> > That can return an error instead of asserting.
> > 
> >> +                    vmstate_info_nullptr.get(f, curr_elem, size, NULL);
> > 
> > You've ignored the return value of the get; that should be  ret = 
> > 
> >> +                } else if (field->flags & VMS_STRUCT) {
> >>                      ret = vmstate_load_state(f, field->vmsd, curr_elem,
> >>                                               field->vmsd->version_id);
> >>                  } else {
> >> @@ -332,7 +336,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, NULL, NULL);
> >> +                } else if (field->flags & VMS_STRUCT) {
> >>                      vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
> >>                  } else {
> >>                      field->info->put(f, curr_elem, size, field, vmdesc_loop);
> >> @@ -747,6 +755,27 @@ const VMStateInfo vmstate_info_uint64 = {
> >>      .put  = put_uint64,
> >>  };
> >>  
> >> +static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
> >> +
> >> +{
> >> +    return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> >> +}
> >> +
> >> +static int put_nullptr(QEMUFile *f, void *pv, size_t size,
> >> +                        VMStateField *field, QJSON *vmdesc)
> >> +
> >> +{
> >> +    assert(pv == NULL);
> >> +    qemu_put_byte(f, VMS_NULLPTR_MARKER);
> >> +    return 0;
> >> +}
> > 
> > Again that assert could just turn into a return -EINVAL
> > 
> > Dave
> > 
> 
> @Dave: I have accidentally answered with 'reply' instead of 'reply all'
> yesterday. Sorry for the noise!
> 
> Thanks for the review! You are right, my code is messy.

It's not that bad - just a few minor issues.

> Yet I'm not sure what you propose is the best way to clean it up. My
> concern is, that we are loosing some info about what exactly went wrong
> (and where), if returning -EINVAL is not combined with additional
> logging/error reporting. 
>
> After re-checking the asserts they all indicate programming errors and
> there is not much (IMHO) client code can do to recover and the user
> should never observe. Unfortunately, I lack knowledge about how is the
> client code handling errors and if there is something we need to clean
> up. I assumed asserting is OK because of the per-existing asserts.

> You are perfectly right about ignoring the return value of
> vmstate_info_nullptr.get and at least I will have to fix that.
> IMHO we have three options to fix this code:
> a) Make vmstate_info_nullptr.get assert too as we expect null pointer
> and we got not null is pretty much an indicator that we can't proceed
> sanely. Possibly add ret = ... for aesthetic reasons. Keep the asserts.
> b) Follow your suggestions without anything on top of it.
> c) Follow your suggestions and add error_report stating what exactly
> went wrong.
> 
> My priority is to get this in, so I will do what you say, or send
> option b) late on Wednesday if no guidance until then.

It's OK to add an error_report as needed; in particular I try and avoid
assert's on the save path where possible, since the user apparently
has a happy running VM, so even if the migration code has an error in it
I'd rather the user didn't lose their VM.  The load path it's OK to
add the asserts since they've not got a working VM yet.

Dave

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

On 02/21/2017 01:22 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>
>>
>> On 02/17/2017 08:42 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.
>>>>
>>>> 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.
>>>>
>>>> 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 in the
>>>> (s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
>>>> an example).
>>>> ---
>>>>  include/migration/vmstate.h |  4 ++++
>>>>  migration/vmstate.c         | 33 +++++++++++++++++++++++++++++++--
>>>>  2 files changed, 35 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>>> index 63e7b02..f2dbf84 100644
>>>> --- a/include/migration/vmstate.h
>>>> +++ b/include/migration/vmstate.h
>>>> @@ -253,6 +253,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 (0x30U) /* '0' */
>>>> +extern const VMStateInfo vmstate_info_nullptr;
>>>> +
>>>>  extern const VMStateInfo vmstate_info_float64;
>>>>  extern const VMStateInfo vmstate_info_cpudouble;
>>>>  
>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>> index 836a7a4..cb81cef 100644
>>>> --- a/migration/vmstate.c
>>>> +++ b/migration/vmstate.c
>>>> @@ -117,7 +117,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);
>>>
>>> That can return an error instead of asserting.
>>>
>>>> +                    vmstate_info_nullptr.get(f, curr_elem, size, NULL);
>>>
>>> You've ignored the return value of the get; that should be  ret = 
>>>
>>>> +                } else if (field->flags & VMS_STRUCT) {
>>>>                      ret = vmstate_load_state(f, field->vmsd, curr_elem,
>>>>                                               field->vmsd->version_id);
>>>>                  } else {
>>>> @@ -332,7 +336,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, NULL, NULL);
>>>> +                } else if (field->flags & VMS_STRUCT) {
>>>>                      vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
>>>>                  } else {
>>>>                      field->info->put(f, curr_elem, size, field, vmdesc_loop);
>>>> @@ -747,6 +755,27 @@ const VMStateInfo vmstate_info_uint64 = {
>>>>      .put  = put_uint64,
>>>>  };
>>>>  
>>>> +static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
>>>> +
>>>> +{
>>>> +    return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
>>>> +}
>>>> +
>>>> +static int put_nullptr(QEMUFile *f, void *pv, size_t size,
>>>> +                        VMStateField *field, QJSON *vmdesc)
>>>> +
>>>> +{
>>>> +    assert(pv == NULL);
>>>> +    qemu_put_byte(f, VMS_NULLPTR_MARKER);
>>>> +    return 0;
>>>> +}
>>>
>>> Again that assert could just turn into a return -EINVAL
>>>
>>> Dave
>>>
>>
>> @Dave: I have accidentally answered with 'reply' instead of 'reply all'
>> yesterday. Sorry for the noise!
>>
>> Thanks for the review! You are right, my code is messy.
> 
> It's not that bad - just a few minor issues.
> 
>> Yet I'm not sure what you propose is the best way to clean it up. My
>> concern is, that we are loosing some info about what exactly went wrong
>> (and where), if returning -EINVAL is not combined with additional
>> logging/error reporting. 
>>
>> After re-checking the asserts they all indicate programming errors and
>> there is not much (IMHO) client code can do to recover and the user
>> should never observe. Unfortunately, I lack knowledge about how is the
>> client code handling errors and if there is something we need to clean
>> up. I assumed asserting is OK because of the per-existing asserts.
> 
>> You are perfectly right about ignoring the return value of
>> vmstate_info_nullptr.get and at least I will have to fix that.
>> IMHO we have three options to fix this code:
>> a) Make vmstate_info_nullptr.get assert too as we expect null pointer
>> and we got not null is pretty much an indicator that we can't proceed
>> sanely. Possibly add ret = ... for aesthetic reasons. Keep the asserts.
>> b) Follow your suggestions without anything on top of it.
>> c) Follow your suggestions and add error_report stating what exactly
>> went wrong.
>>
>> My priority is to get this in, so I will do what you say, or send
>> option b) late on Wednesday if no guidance until then.
> 
> It's OK to add an error_report as needed; in particular I try and avoid
> assert's on the save path where possible, since the user apparently
> has a happy running VM, so even if the migration code has an error in it
> I'd rather the user didn't lose their VM.  The load path it's OK to
> add the asserts since they've not got a working VM yet.
> 
> Dave
> 

You are right, distinguishing between save and load paths makes a lot
of sense. So on the load path I will use asserts, and on the save path,
that is

static int put_nullptr(QEMUFile *f, void *pv, size_t size,
                        VMStateField *field, QJSON *vmdesc)

{

here, I could do:
-    assert(pv == NULL);
+    if (pv != NULL) {
+        error_report("put_nullptr must be called with pv == NULL");
+        return -EINVAL; 
+    }
    qemu_put_byte(f, VMS_NULLPTR_MARKER);
    return 0;
}

And of course, I will fix ignoring the return value too. Would that be
satisfactory? If not, please complain.

By the way, I could also omit the check on pv, and ignore pv altogether.
A call to put_nullptr means we encountered a nullptr and the check
is just for catching buggy client code -- what might be an overkill
in this case.

Thank you very much. FYI I intend to send out the next version tomorrow.

Regards,
Halil

On 02/21/2017 01:55 PM, Halil Pasic wrote:
>> It's OK to add an error_report as needed; in particular I try and avoid
>> assert's on the save path where possible, since the user apparently
>> has a happy running VM, so even if the migration code has an error in it
>> I'd rather the user didn't lose their VM.  The load path it's OK to
>> add the asserts since they've not got a working VM yet.
>>
>> Dave
>>
> You are right, distinguishing between save and load paths makes a lot
> of sense. So on the load path I will use asserts, and on the save path,
> that is
> 
> static int put_nullptr(QEMUFile *f, void *pv, size_t size,
>                         VMStateField *field, QJSON *vmdesc)
> 
> {
> 
> here, I could do:
> -    assert(pv == NULL);
> +    if (pv != NULL) {
> +        error_report("put_nullptr must be called with pv == NULL");
> +        return -EINVAL; 
> +    }
>     qemu_put_byte(f, VMS_NULLPTR_MARKER);
>     return 0;
> }
> 
> And of course, I will fix ignoring the return value too. Would that be
> satisfactory? If not, please complain.
> 
> By the way, I could also omit the check on pv, and ignore pv altogether.
> A call to put_nullptr means we encountered a nullptr and the check
> is just for catching buggy client code -- what might be an overkill
> in this case.
> 
> Thank you very much. FYI I intend to send out the next version tomorrow.
> 
> Regards,
> Halil
> 
> 
> 

Just realized two things:
1. vmstate_save_state does not have a return value and does not handle
such. So I won't fail the migration from put_nullptr.
2. Since we have no rule on NDEBUG, I think  I should not rely on assert doing
anything. So where important for correctness I will do return value and
error_report.