On Wed, Mar 06, 2024 at 09:25:24AM +0000, Daniel P. Berrangé wrote:
> On Wed, Mar 06, 2024 at 08:52:41AM +0800, Peter Xu wrote:
> > On Tue, Mar 05, 2024 at 05:49:33PM +0000, Daniel P. Berrangé wrote:
> > > I don't think you should be removing this. Calling qio_channel_close()
> > > remains recommended best practice, even with fdatasync() removed, as
> > > it provides a strong guarantee that the FD is released which you don't
> > > get if you rely on the ref count being correctly decremented in all
> > > code paths.
> >
> > Hmm, I have the confusion on why ioc->fd is more special than the ioc
> > itself when leaked. It'll be a bug anyway if we leak any of them? Leaking
> > fds may also help us to find such issue easier (e.g. by seeing stale fds
> > under /proc). From that POV I tend to agree on the original proposal.
>
> Closing the FD would cause any registered I/O handlers callbacks to
> get POLLNVAL and may well trigger cleanup that will prevent the leak.
It's not possible anymore that we will have such handler callbacks when
reaching here, am I right? AFAIU that's my understanding after commit
9221e3c6a2 ("migration/multifd: Cleanup TLS iochannel referencing").
Would it be possible if we can assert that fact (either on "there's no
handler callback", or "we're the last reference" then it implies no
handlers) rather than doing an explicit close() (and if we do the latter,
we'd better explain the POLLNVAL bits)?
Thanks,
--
Peter Xu
