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

[Daniel P . Berrangé]

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é <address@hidden> 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é <address@hidden> 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" <address@hidden> wrote:
> > > > >     
> > > > > > * Lukas Straub (address@hidden) 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 <address@hidden>      
> > > > > >     
> > > > > > > +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 :|