Re: [PATCH 3/5] block/nbd.c: Add yank feature

[Lukas Straub <lukasstraub2@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]

On Tue, 12 May 2020 09:54:58 +0100
Daniel P. Berrangé <berrange@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" <dgilbert@redhat.com> wrote:
> >   
> > > * Lukas Straub (lukasstraub2@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 <lukasstraub2@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]