[Libguestfs] [PATCH libnbd 1/4] api: Combine callback and user_data into a single struct.
Eric Blake
eblake at redhat.com
Wed Aug 14 03:06:06 UTC 2019
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>
More information about the Libguestfs
mailing list