On Fri, May 15, 2020 at 12:14:47PM +0200, Lukas Straub wrote: > On Fri, 15 May 2020 11:04:13 +0100 > Daniel P. Berrangé <berra...@redhat.com> wrote: > > > 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. > > Hmm, but if object_unref frees the object, it doesn't set the > pointer to NULL does it?
So set "ioc = NULL" after calling object_unref. AFAICT, nbd already does exactly this. 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 :|