Libguestfs - Aug 2019 - Re: [PATCH libnbd 1/4] api: Combine callback and user_data into a single struct.

On 8/13/19 5:36 PM, Richard W.M. Jones wrote:
> The definition of functions that take a callback is changed so that
> the callback and user_data are combined into a single structure, eg:
> 
>   int64_t nbd_aio_pread (struct nbd_handle *h,
>             void *buf, size_t count, uint64_t offset,
> -           int (*completion_callback) (/*..*/), void *user_data,
> +           nbd_completion_callback completion_callback,
>             uint32_t flags);
> 
> Several nbd_*_callback structures are defined.  The one corresponding
> to the example above is:
> 
>   typedef struct {
>     void *user_data;
>     int (*callback) (unsigned valid_flag, void *user_data, int *error);
>   } nbd_completion_callback;
> 
> The nbd_aio_pread function can now be called using:
> 
>   nbd_aio_pread (nbd, buf, sizeof buf, offset,
>                  (nbd_completion_callback) { .callback = my_fn,
>                                              .user_data = my_data },
Is it worth arranging the C struct to match the typical argument
ordering of user_data last?  It doesn't make any real difference (the
struct size doesn't change, and the compiler handles out-of-order member
initialization just fine), but doing it could allow the use of:

nbd_completion_callback cb = { my_fn, my_data };
nbd_aio_pread (nbd, buf, sizeof buf, offset, cb, 0);

where the omission of .member= designators may result in less typing,
but only if the member order matches.
> +++ b/docs/libnbd.pod
> @@ -598,14 +598,25 @@ will use your login name):
>  
>  =head1 CALLBACKS
>  
> -Some libnbd calls take function pointers (eg.
> -C<nbd_set_debug_callback>, C<nbd_aio_pread>).  Libnbd can call
these
> -functions while processing.
> -
> -Callbacks have an opaque C<void *user_data> pointer.  This is passed
> -as the second parameter to the callback.  The opaque pointer is only
> -used from the C API, since in other languages you can use closures to
> -achieve the same outcome.
> +Some libnbd calls take callbacks (eg.  C<nbd_set_debug_callback>,
2 spaces looks odd (emacs thinks eg. ended a sentence)
> +C<nbd_aio_pread>).  Libnbd can call these functions while
processing.
> +
> +In the C API these libnbd calls take a structure which contains the
> +function pointer and an optional opaque C<void *user_data> pointer:
> +
> + nbd_aio_pread (nbd, buf, sizeof buf, offset,
> +                (nbd_completion_callback) { .callback = my_fn,
> +                                            .user_data = my_data },
> +                0);
> +
> +If you don't want the callback, either set C<.callback> to
C<NULL> or
Maybe: s/If/For optional callbacks, if/

(coupled with the observation made earlier today that we still want a
followup patch to better document Closure vs. OClosure on which
callbacks do not allow a NULL fn in libnbd-api.3).
> +++ b/examples/batched-read-write.c
> @@ -53,12 +53,13 @@ try_deadlock (void *arg)
>  
>    /* Issue commands. */
>    cookies[0] = nbd_aio_pread (nbd, in, packetsize, 0,
> -                              NULL, NULL, 0);
> +                              NBD_NULL_CALLBACK(completion), 0);
A bit more verbose, but the macro cuts it down from something even
longer to type.  I can live with this conversion.
> +++ b/examples/glib-main-loop.c
> @@ -384,7 +384,8 @@ read_data (gpointer user_data)
>  
>    if (nbd_aio_pread (gssrc->nbd, buffers[i].data,
>                       BUFFER_SIZE, buffers[i].offset,
> -                     finished_read, &buffers[i], 0) == -1) {
> +                     (nbd_completion_callback) { .callback =
finished_read, .user_data = &buffers[i] },
> +                     0) == -1) {
>      fprintf (stderr, "%s\n", nbd_get_error ());
>      exit (EXIT_FAILURE);
>    }
> @@ -428,7 +429,8 @@ write_data (gpointer user_data)
>    buffer->state = BUFFER_WRITING;
>    if (nbd_aio_pwrite (gsdest->nbd, buffer->data,
>                        BUFFER_SIZE, buffer->offset,
> -                      finished_write, buffer, 0) == -1) {
> +                      (nbd_completion_callback) { .callback =
finished_write, .user_data = buffer },
Worth splitting the long lines?
> +++ b/interop/structured-read.c
> @@ -147,7 +147,8 @@ main (int argc, char *argv[])
>  
>    memset (rbuf, 2, sizeof rbuf);
>    data = (struct data) { .count = 2, };
> -  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, read_cb,
&data,
> +  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048,
> +                            (nbd_chunk_callback) { .callback = read_cb,
.user_data = &data },
>                              0) == -1) {
>      fprintf (stderr, "%s\n", nbd_get_error ());
>      exit (EXIT_FAILURE);
> @@ -157,7 +158,8 @@ main (int argc, char *argv[])
>    /* Repeat with DF flag. */
>    memset (rbuf, 2, sizeof rbuf);
>    data = (struct data) { .df = true, .count = 1, };
> -  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, read_cb,
&data,
> +  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048,
> +                            (nbd_chunk_callback) { .callback = read_cb,
.user_data = &data },
When reusing the same (nbd_chunk_callback) more than once, we could
hoist it into a helper variable to initialize it only once rather than
repeating ourselves. Not essential to this patch (I like that you
remained mechanical), but could be done as a later cleanup if desired.

ACK

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Aug 13, 2019 at 10:06:06PM -0500, Eric Blake
wrote:
> On 8/13/19 5:36 PM, Richard W.M. Jones wrote:
> > The definition of functions that take a callback is changed so that
> > the callback and user_data are combined into a single structure, eg:
> > 
> >   int64_t nbd_aio_pread (struct nbd_handle *h,
> >             void *buf, size_t count, uint64_t offset,
> > -           int (*completion_callback) (/*..*/), void *user_data,
> > +           nbd_completion_callback completion_callback,
> >             uint32_t flags);
> > 
> > Several nbd_*_callback structures are defined.  The one corresponding
> > to the example above is:
> > 
> >   typedef struct {
> >     void *user_data;
> >     int (*callback) (unsigned valid_flag, void *user_data, int
*error);
> >   } nbd_completion_callback;
> > 
> > The nbd_aio_pread function can now be called using:
> > 
> >   nbd_aio_pread (nbd, buf, sizeof buf, offset,
> >                  (nbd_completion_callback) { .callback = my_fn,
> >                                              .user_data = my_data },
> 
> Is it worth arranging the C struct to match the typical argument
> ordering of user_data last?  It doesn't make any real difference (the
> struct size doesn't change, and the compiler handles out-of-order
member
> initialization just fine), but doing it could allow the use of:
> 
> nbd_completion_callback cb = { my_fn, my_data };
> nbd_aio_pread (nbd, buf, sizeof buf, offset, cb, 0);
> 
> where the omission of .member= designators may result in less typing,
> but only if the member order matches.
Sure.  What would be the arrangement when all 3 members are
present?  I guess { callback, user_data, free }?
> > +++ b/docs/libnbd.pod
> > @@ -598,14 +598,25 @@ will use your login name):
> >  
> >  =head1 CALLBACKS
> >  
> > -Some libnbd calls take function pointers (eg.
> > -C<nbd_set_debug_callback>, C<nbd_aio_pread>).  Libnbd can
call these
> > -functions while processing.
> > -
> > -Callbacks have an opaque C<void *user_data> pointer.  This is
passed
> > -as the second parameter to the callback.  The opaque pointer is only
> > -used from the C API, since in other languages you can use closures to
> > -achieve the same outcome.
> > +Some libnbd calls take callbacks (eg. 
C<nbd_set_debug_callback>,
> 
> 2 spaces looks odd (emacs thinks eg. ended a sentence)
OK fixed in my copy.

Interesting fact about emacs I-search: It doesn't let you search for
double spaces for some reason.
> > +++ b/examples/batched-read-write.c
> > @@ -53,12 +53,13 @@ try_deadlock (void *arg)
> >  
> >    /* Issue commands. */
> >    cookies[0] = nbd_aio_pread (nbd, in, packetsize, 0,
> > -                              NULL, NULL, 0);
> > +                              NBD_NULL_CALLBACK(completion), 0);
> 
> A bit more verbose, but the macro cuts it down from something even
> longer to type.  I can live with this conversion.
I was trying to write a generic macro (ie. just ‘NBD_NULL_CALLBACK’)
for this, but I don't believe it's possible.
> > +++ b/examples/glib-main-loop.c
> > @@ -384,7 +384,8 @@ read_data (gpointer user_data)
> >  
> >    if (nbd_aio_pread (gssrc->nbd, buffers[i].data,
> >                       BUFFER_SIZE, buffers[i].offset,
> > -                     finished_read, &buffers[i], 0) == -1) {
> > +                     (nbd_completion_callback) { .callback =
finished_read, .user_data = &buffers[i] },
> > +                     0) == -1) {
> >      fprintf (stderr, "%s\n", nbd_get_error ());
> >      exit (EXIT_FAILURE);
> >    }
> > @@ -428,7 +429,8 @@ write_data (gpointer user_data)
> >    buffer->state = BUFFER_WRITING;
> >    if (nbd_aio_pwrite (gsdest->nbd, buffer->data,
> >                        BUFFER_SIZE, buffer->offset,
> > -                      finished_write, buffer, 0) == -1) {
> > +                      (nbd_completion_callback) { .callback =
finished_write, .user_data = buffer },
> 
> Worth splitting the long lines?
It's tricky to format these.  Even after splitting the line they still
go over 7x chars.

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

On 8/14/19 4:21 AM, Richard W.M. Jones wrote:
>>> The nbd_aio_pread function can now be called using:
>>>
>>>   nbd_aio_pread (nbd, buf, sizeof buf, offset,
>>>                  (nbd_completion_callback) { .callback = my_fn,
>>>                                              .user_data = my_data
},
>>
>> Is it worth arranging the C struct to match the typical argument
>> ordering of user_data last?  It doesn't make any real difference
(the
>> struct size doesn't change, and the compiler handles out-of-order
member
>> initialization just fine), but doing it could allow the use of:
>>
>> nbd_completion_callback cb = { my_fn, my_data };
>> nbd_aio_pread (nbd, buf, sizeof buf, offset, cb, 0);
>>
>> where the omission of .member= designators may result in less typing,
>> but only if the member order matches.
> 
> Sure.  What would be the arrangement when all 3 members are
> present?  I guess { callback, user_data, free }?
Yes, I think that's the most convenient arrangement based on which
fields are most likely to be left NULL.

>>> +Some libnbd calls take callbacks (eg. 
C<nbd_set_debug_callback>,
>>
>> 2 spaces looks odd (emacs thinks eg. ended a sentence)
> 
> OK fixed in my copy.
> 
> Interesting fact about emacs I-search: It doesn't let you search for
> double spaces for some reason.
Yeah, emacs intentionally treats all runs of 1 or more spaces in text as
a match to single space in I-search. It's often nicer that way, but does
sometimes get in the way; Regexp I-search with '[ ]' is my trick for
finding an exact space.
> 
>>> +++ b/examples/batched-read-write.c
>>> @@ -53,12 +53,13 @@ try_deadlock (void *arg)
>>>  
>>>    /* Issue commands. */
>>>    cookies[0] = nbd_aio_pread (nbd, in, packetsize, 0,
>>> -                              NULL, NULL, 0);
>>> +                              NBD_NULL_CALLBACK(completion), 0);
>>
>> A bit more verbose, but the macro cuts it down from something even
>> longer to type.  I can live with this conversion.
> 
> I was trying to write a generic macro (ie. just ‘NBD_NULL_CALLBACK’)
> for this, but I don't believe it's possible.
Thinking about this more:

As written, your patch allows:

nbd_set_debug_callback(NBD_NULL_CALLBACK(debug))

to compile, even though it will fail at runtime.  The problem? Your
macro is too generic.  Better would be if we had a macro per type used
in OClosure (and no macro for types used only as Closure), where the
decision is made at generator time rather than at C compilation time
(the presence or absence of a macro then becomes a useful witness of
whether that type of closure is optional).

Taking this idea further, of our 4 callback types, only one is used as
OClosure.  For full genericity, we could enhance the generator to check
which 'closure' instances are used where, and if used as an OClosure,
then rely on String.uppercase_ascii closure.cbname to generate the
appropriate C macro.  Or, since we know we only have one such type, it's
even faster to just hard-code a one-line addition to libnbd.h:

#define NBD_NULL_COMPLETION (nbd_completion_callback) { NULL }

or maybe

#define NBD_NULL_COMPLETION \
  (nbd_completion_callback) { .callback = NULL, .user_data = NULL }

depending on how likely it is that someone might be using our header
with a compiler mode that warns on an initializer that doesn't fully
specify everything.

Then, whether we go with a generic generator fix, or an open-coded
one-liner, the rest of your patch changes via:

s/NBD_NULL_CALLBACK(completion)/NBD_NULL_COMPLETION/

>>> @@ -428,7 +429,8 @@ write_data (gpointer user_data)
>>>    buffer->state = BUFFER_WRITING;
>>>    if (nbd_aio_pwrite (gsdest->nbd, buffer->data,
>>>                        BUFFER_SIZE, buffer->offset,
>>> -                      finished_write, buffer, 0) == -1) {
>>> +                      (nbd_completion_callback) { .callback =
finished_write, .user_data = buffer },
>>
>> Worth splitting the long lines?
> 
> It's tricky to format these.  Even after splitting the line they still
> go over 7x chars.
Would it ease typing if we added a macro:

NBD_COMPLETION (finished_read, &buffers[i])

although that gets trickier when we add an optional free callback (C
already allows initializers with 1, 2, or 3 members set, but writing a
variable-argument macro to do the same gets rather hairy - it can be
done with the help of intermediate macros, but that's a lot to cram into
libnbd.h).

So I guess we just let this form stay longhand.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org