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.
