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; } If you don't want the code in QIOChannel I guess I can create a subclass (YankableChannelSocket?) of QIOChannelSocket. What do you think? Regards, Lukas Straub > This will make the NBD code clearer too, as we can see exactly > what actions are performed at NBD layer when a yank happens, > which is what David was not seeing clearly here. > > Regards, > Daniel
pgp2r3jw_rKwe.pgp
Description: OpenPGP digital signature