Re: [PATCH v3 18/24] vfio/migration: Don't run load cleanup if load setup didn't run

[Maciej S. Szmigiero]

On 29.11.2024 15:08, Cédric Le Goater wrote:
> On 11/17/24 20:20, Maciej S. Szmigiero wrote:
> > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> >
> > It's possible for load_cleanup SaveVMHandler to get called without
> > load_setup handler being called first.
> >
> > Since we'll be soon running cleanup operations there that access objects
> > that need earlier initialization in load_setup let's make sure these
> > cleanups only run when load_setup handler had indeed been called
> > earlier.
> >
> > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>
> tbh, that's a bit ugly. I agree it's similar to those 'bool initialized'
> attributes we have in some structs, so nothing new or really wrong.
> But it does look like a workaound for a problem or cleanups missing
> that would need time to untangle.
>
> I would prefer to avoid this change and address the issue from the
> migration subsystem if possible.

While it would be pretty simple to only call {load,save}_cleanup
SaveVMHandlers when the relevant {load,save}_setup handler was
successfully called first this would amount to a change of these
handler semantics.

This would risk introducing regressions - for example vfio_save_setup()
doesn't clean up (free) newly allocated migration->data_buffer
if vfio_migration_set_state() were to fail later in this handler
and relies on an unconstitutional call to vfio_save_cleanup() in
order to clean it up.

There might be similar issues in other drivers too.

> Thanks,
>
> C.

Thanks,
Maciej