On Fri, May 15, 2020 at 11:48:18AM +0200, Lukas Straub wrote:
> On Tue, 12 May 2020 09:54:58 +0100
> Daniel P. Berrangé <berra...@redhat.com> wrote:
>
> > On Mon, May 11, 2020 at 07:05:24PM +0200, Lukas Straub wrote:
> > > On Mon, 11 May 2020 17:19:09 +0100
> > > "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> > >
> > > > * Lukas Straub (lukasstra...@web.de) wrote:
> > > > > Add yank option, pass it to the socket-channel and register a yank
> > > > > function which sets s->state = NBD_CLIENT_QUIT. This is the same
> > > > > behaviour as if an error occured.
> > > > >
> > > > > Signed-off-by: Lukas Straub <lukasstra...@web.de>
> > > >
> > > > > +static void nbd_yank(void *opaque)
> > > > > +{
> > > > > + BlockDriverState *bs = opaque;
> > > > > + BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> > > > > +
> > > > > + atomic_set(&s->state, NBD_CLIENT_QUIT);
> > > >
> > > > I think I was expecting a shutdown on the socket here - why doesn't it
> > > > have one?
> > >
> > > For nbd, we register two yank functions: This one and we enable
> > > the yank feature on the qio channel (see function
> > > nbd_establish_connection below).
> >
> > As mentioned on the earlier patch, I don't want to see any yank
> > code in the QIOChannel object directly. This nbd_yank function
> > can simply call the qio_channel_shutdown() function directly
> > and avoid need for modifying the QIOChannel object with yank
> > support.
>
> Hi,
> Looking at it again, the problem is not with registering the yank functions,
> but with tracking the lifetime of it. Suppose we add qio_channel_shutdown to
> the yank_nbd function. Then we need to unregister it whenever the QIOChannel
> object is freed.
>
> In the code that would lead to the following constructs in a lot of places:
> if (local_err) {
> yank_unregister_function(s->yank_name, yank_nbd, bs);
> object_unref(OBJECT(sioc));
> error_propagate(errp, local_err);
> return NULL;
> }
The nbd patch here already has a yank_unregister_function() so I'm
not seeing anything changes in that respect. The "yank_nbd" function
should check that the I/O channel is non-NULL before calling the
qio_channel_shutdown method.
> If you don't want the code in QIOChannel I guess I can create a
> subclass (YankableChannelSocket?) of QIOChannelSocket. What do
> you think?
That's no better, and I don't see any compelling need for it as calling
yank_unregister_function is something nbd already does in its nbd_close
function. It isn't a burden for the other backends to do similarly.
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 :|
