Re: [PATCH] migration: Ensure vmstate_save() sets errp

[Hanna Czenczek <hreitz@redhat.com>]

On 15.10.24 18:06, Peter Xu wrote:
> On Tue, Oct 15, 2024 at 04:15:15PM +0200, Hanna Czenczek wrote:
>> migration/savevm.c contains some calls to vmstate_save() that are
>> followed by migrate_set_error() if the integer return value indicates an
>> error.  migrate_set_error() requires that the `Error *` object passed to
>> it is set.  Therefore, vmstate_save() is assumed to always set *errp on
>> error.
>>
>> Right now, that assumption is not met: vmstate_save_state_v() (called
>> internally by vmstate_save()) will not set *errp if
>> vmstate_subsection_save() or vmsd->post_save() fail.  Fix that by adding
>> an *errp parameter to vmstate_subsection_save(), and by generating a
>> generic error in case post_save() fails (as is already done for
>> pre_save()).
>>
>> Without this patch, qemu will crash after vmstate_subsection_save() or
>> post_save() have failed inside of a vmstate_save() call (unless
>> migrate_set_error() then happen to discard the new error because
>> s->error is already set).  This happens e.g. when receiving the state
>> from a virtio-fs back-end (virtiofsd) fails.
>>
>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
>> ---
>>   migration/vmstate.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index ff5d589a6d..13532f2807 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -22,7 +22,8 @@
>>   #include "trace.h"
>>   
>>   static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>> -                                   void *opaque, JSONWriter *vmdesc);
>> +                                   void *opaque, JSONWriter *vmdesc,
>> +                                   Error **errp);
>>   static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>>                                      void *opaque);
>>   
>> @@ -441,12 +442,13 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>>           json_writer_end_array(vmdesc);
>>       }
>>   
>> -    ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc);
>> +    ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
>>   
>>       if (vmsd->post_save) {
>>           int ps_ret = vmsd->post_save(opaque);
>>           if (!ret) {
> Perhaps here it needs to be "if (!ret && ps_ret)" now, otherwise the error
> will be attached even if no error for both retvals?

Not just perhaps, that seems kind of vital indeed...

(I blame my brain’s less-than-stellar pattern matching that might have 
interpreted this as the error path *cough*)

> Other than that it looks good.

Thanks for the quick review!  Will respin.

Hanna

>
> Thanks,
>
>>               ret = ps_ret;
>> +            error_setg(errp, "post-save failed: %s", vmsd->name);
>>           }
>>       }
>>       return ret;
>> @@ -518,7 +520,8 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>>   }
>>   
>>   static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>> -                                   void *opaque, JSONWriter *vmdesc)
>> +                                   void *opaque, JSONWriter *vmdesc,
>> +                                   Error **errp)
>>   {
>>       const VMStateDescription * const *sub = vmsd->subsections;
>>       bool vmdesc_has_subsections = false;
>> @@ -546,7 +549,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>>               qemu_put_byte(f, len);
>>               qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len);
>>               qemu_put_be32(f, vmsdsub->version_id);
>> -            ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc);
>> +            ret = vmstate_save_state_with_err(f, vmsdsub, opaque, vmdesc, errp);
>>               if (ret) {
>>                   return ret;
>>               }
>> -- 
>> 2.45.2
>>