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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190813/a776571b/attachment.sig>