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? > > 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
pgp3SRe0WPf6M.pgp
Description: OpenPGP digital signature