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 <pet...@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
