Re: [PATCH v4 8/8] migration: Add a wrapper to cleanup migration files

[Peter Xu]

On Wed, Aug 16, 2023 at 03:35:24PM -0400, Peter Xu wrote:
> On Wed, Aug 16, 2023 at 03:47:26PM -0300, Fabiano Rosas wrote:
> > Peter Xu <peterx@redhat.com> writes:
> > 
> > > On Wed, Aug 16, 2023 at 11:25:10AM -0300, Fabiano Rosas wrote:
> > >> @@ -2003,6 +1980,8 @@ static int 
> > >> open_return_path_on_source(MigrationState *ms)
> > >>          return -1;
> > >>      }
> > >>  
> > >> +    
> > >> migration_ioc_register_yank(qemu_file_get_ioc(ms->rp_state.from_dst_file));
> > >
> > > I think I didn't really get why it wasn't paired before yesterday.  My
> > > fault.
> > >
> > > Registering from_dst_file, afaict, will register two identical yank 
> > > objects
> > > because the ioc is the same.
> > >
> > 
> > Why do we have two QEMUFiles for the same fd again?
> 
> Because qemufile has a "direction" (either read / write)?
> 
> > 
> > We're bound to crash at some point by trying to qemu_fclose() the two
> > QEMUFiles at the same time.
> 
> Even with each qemufile holding a reference on the ioc object?  I thought
> it won't crash, but if it will please point that out; or fix it would be
> even better.
> 
> > 
> > > Should we make migration_file_release() not handle the unregister of
> > > yank(), but leave that to callers?  Then we keep the rule of only register
> > > yank for each ioc once.
> > >
> > 
> > We need the unregister to be at migration_file_release() so that it
> > takes benefit of the locking while checking the file for NULL. If it
> > moves out then the caller will have to do locking as well. Which
> > defeats the purpose of the patch.
> > 
> > I don't understand why you moved the unregister out of channel_close in
> > commit 39675ffffb ("migration: Move the yank unregister of channel_close
> > out"). You called it a "hack" at the time, but looking at the current
> > situation, it seems like a reasonable thing to do: unregister the yank
> > when the ioc refcount drops to 1.
> > 
> > I would go even further and say that qemu_fclose should also avoid
> > calling qio_channel_close if the ioc refcnt is elevated.
> 
> I'd rather not; I still think it's a hack, always open to be corrected.
> 
> I think the problem is yank can register anything so it's separate from
> iochannels.  If one would like to have ioc close() automatically
> unregister, then one should also register yank transparently without the
> ioc user even aware of yank's existance.
> 
> Now the condition is the caller register yank itself, then I think the
> caller should unreg it.. not iochannel itself, silently.

I just noticed this is not really copying the list.. let me add the cc list
back, assuming it was just forgotten.

One more thing to mention is, now I kind of agree probably we should
register yank over each qemufile, as you raised the concern in the other
thread that otherwise qmp_yank() won't set error for the qemufile, which
seems to be unexpected.

-- 
Peter Xu