[Libguestfs] [PATCH libnbd v2 3/3] api: Add nbd_clear_debug_callback.

[Richard W.M. Jones]

On Tue, Aug 13, 2019 at 10:41:27AM -0500, Eric Blake wrote:
> On 8/13/19 10:12 AM, Richard W.M. Jones wrote:
> > Commit 0904dd113dfa36485623b3c1756dc1aeab3dddeb removed the
> > theoretical possibility of clearing the debug callback from the handle
> > by setting it to NULL (although that was dubious from an API point of
> > view).
> > 
> > This commit adds an explicit way to do this.  Another advantage of
> > this is we can internally call this API from other places when we want
> > to clear/free the debug callback.
> > ---
> >  generator/generator | 11 +++++++++++
> >  lib/debug.c         | 17 ++++++++++++++---
> >  lib/handle.c        |  4 +---
> >  3 files changed, 26 insertions(+), 6 deletions(-)
> > 
> > diff --git a/generator/generator b/generator/generator
> > index 6ccfc5f..d76eeea 100755
> > --- a/generator/generator
> > +++ b/generator/generator
> > @@ -1013,6 +1013,17 @@ a handle then messages are printed on C<stderr>.
> >  
> >  The callback should not call C<nbd_*> APIs on the same handle since it can
> >  be called while holding the handle lock and will cause a deadlock.";
> > +};
> > +
> > +  "clear_debug_callback", {
> > +    default_call with
> > +    args = [];
> > +    ret = RErr;
> 
> Is RErr the right type, or can we make this a 'never fails' function?

Yes this can never fail.  We don't have a way to say that (except
RUInt but that's a bit different), and I'm a bit dubious about adding
one because it limits our choices in future.

> > +    shortdesc = "clear the debug callback";
> > +    longdesc = "\
> > +Remove the debug callback if one was previously associated
> > +with the handle (with C<nbd_set_debug_callback>).  If not
> 
> s/not/no/

Oops, fixed in my copy.

> > +callback was associated this does nothing.";
> >  };
> >  
> >    "set_handle_name", {
> > diff --git a/lib/debug.c b/lib/debug.c
> > index ad4d9cb..c1decb2 100644
> > --- a/lib/debug.c
> > +++ b/lib/debug.c
> > @@ -38,13 +38,24 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
> >    return h->debug;
> >  }
> >  
> > +int
> > +nbd_unlocked_clear_debug_callback (struct nbd_handle *h)
> > +{
> > +  if (h->debug_callback)
> > +    /* ignore return value */
> > +    h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
> > +  h->debug_callback = NULL;
> > +  h->debug_data = NULL;
> > +  return 0;
> > +}
> 
> Is it worth returning -1 if no callback was associated, and/or
> forwarding the return value of callback(FREE, ptr) as the return value
> of this function?  (That's more complicated; I'm also fine with always
> returning 0 and ignoring the callback(FREE) return).

I think it's probably best to return 0 always.  Reduces complexity for
callers if they don't have to worry if a debug function was previously
registered or not.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org