Re: [PATCH v3 10/10] migration: Add a wrapper to cleanup migration files

[Peter Xu <peterx@redhat.com>]

On Fri, Aug 11, 2023 at 12:08:36PM -0300, Fabiano Rosas wrote:
> We currently have a pattern for cleaning up a migration QEMUFile:
> 
>   qemu_mutex_lock(&s->qemu_file_lock);
>   file = s->file_name;
>   s->file_name = NULL;
>   qemu_mutex_unlock(&s->qemu_file_lock);
> 
>   migration_ioc_unregister_yank_from_file(file);
>   qemu_file_shutdown(file);
>   qemu_fclose(file);
> 
> There are some considerations for this sequence:
> 
> - we must clear the pointer under the lock, to avoid TOC/TOU bugs;
> - the shutdown() and close() expect be given a non-null parameter;
> - a close() in one thread should not race with a shutdown() in another;
> 
> Create a wrapper function to make sure everything works correctly.
> 
> Note: the return path did not used to call
>       migration_ioc_unregister_yank_from_file(), but I added it
>       nonetheless for uniformity.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

This definitely looks cleaner.  Probably can be squashed together with
previous patch?  If you could double check whether we can just drop the
shutdown() all over the places when close() altogether, it'll be even
nicer (I hope I didn't miss any real reasons to explicitly do that).

> diff --git a/util/yank.c b/util/yank.c
> index abf47c346d..4b6afbf589 100644
> --- a/util/yank.c
> +++ b/util/yank.c
> @@ -146,8 +146,6 @@ void yank_unregister_function(const YankInstance *instance,
>              return;
>          }
>      }
> -
> -    abort();

I think we can't silently do this.  This check is very strict and I guess
you removed it because you hit a crash.  What's the crash?  Can we just
pair the yank reg/unreg?

>  }
>  
>  void qmp_yank(YankInstanceList *instances,
> -- 
> 2.35.3
> 

-- 
Peter Xu