Re: [PATCH 1/2] io: move websock resource release to close method

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Sep 30, 2025 at 03:21:10PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Sep 30, 2025 at 3:08 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > The QIOChannelWebsock object releases all its resources in the
> > finalize callback. This is too late, as callers expect to be
> > able to call qio_channel_close() to fully close a channel and
> > release resources related to I/O. Only releasing the underlying
> > QIOChannel transport can be delayed until finalize.
> >
> > Furthermore the close callback must be robust against being
> > called multiple times. Thus when moving the code we now clear
> > the GSource ID using  g_clear_handle_id.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  io/channel-websock.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/io/channel-websock.c b/io/channel-websock.c
> > index 0a8c5c4712..56d53355d5 100644
> > --- a/io/channel-websock.c
> > +++ b/io/channel-websock.c
> > @@ -919,16 +919,7 @@ static void qio_channel_websock_finalize(Object *obj)
> >  {
> >      QIOChannelWebsock *ioc = QIO_CHANNEL_WEBSOCK(obj);
> >
> > -    buffer_free(&ioc->encinput);
> > -    buffer_free(&ioc->encoutput);
> > -    buffer_free(&ioc->rawinput);
> >      object_unref(OBJECT(ioc->master));
> > -    if (ioc->io_tag) {
> > -        g_source_remove(ioc->io_tag);
> > -    }
> > -    if (ioc->io_err) {
> > -        error_free(ioc->io_err);
> > -    }
> 
> Maybe finalize should call close() ? Otherwise, it's hard to guarantee
> that there is no leak when doing simply init/finish.

I was in two minds about that. If close did happen implicitly during
finalize that is highly likely to be a bug in the usage, as you should
not finalize a I/O channel that is still in use. This patch matches the
approach we've taken in the TLS channel now.

> 
> >  }
> >
> >
> > @@ -1218,6 +1209,15 @@ static int qio_channel_websock_close(QIOChannel *ioc,
> >      QIOChannelWebsock *wioc = QIO_CHANNEL_WEBSOCK(ioc);
> >
> >      trace_qio_channel_websock_close(ioc);
> > +    buffer_free(&wioc->encinput);
> > +    buffer_free(&wioc->encoutput);
> > +    buffer_free(&wioc->rawinput);
> > +    if (wioc->io_tag) {
> > +        g_clear_handle_id(&wioc->io_tag, g_source_remove);
> > +    }
> > +    if (wioc->io_err) {
> > +        g_clear_pointer(&wioc->io_err, error_free);
> > +    }
> >      return qio_channel_close(wioc->master, errp);
> >  }
> >
> 
> otherwise lgtm
> 
> > --
> > 2.50.1
> >
> 
> 
> -- 
> Marc-André Lureau
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|