Re: [Qemu-devel] [PATCH 3/5] migration/vmstate: fix array of ptr with nullptrs

[Halil Pasic]

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.