Re: [Libguestfs] [PATCH libnbd 1/4] api: Combine callback and user_data into a single struct.
On Wed, Aug 14, 2019 at 06:44:37AM -0500, Eric Blake wrote:
> #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/
OK I'll fix this.
> Would it ease typing if we added a macro:
>
> NBD_COMPLETION (finished_read, [i])
This is not really much different from:
(nbd_completion_callback) { finished_read, [i] }
which is fine to use now that I've rearranged the struct fields
(in my local copy).
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd 1/4] api: Combine callback and user_data into a single struct.
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, C). Libnbd can call these
> > -functions while processing.
> > -
> > -Callbacks have an opaque C 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,
>
> 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, [i], 0) == -1) {
> > + (nbd_completion_callback) { .callback =
> > finished_read, .user_data = [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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [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, C). Libnbd can call these
> -functions while processing.
> -
> -Callbacks have an opaque C 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,
2 spaces looks odd (emacs thinks eg. ended a sentence)
> +C). 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 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 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, [i], 0) == -1) {
> + (nbd_completion_callback) { .callback = finished_read,
> .user_data = [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, ,
> + if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048,
> +(nbd_chunk_callback) { .callback = read_cb,
> .user_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, ,
> + if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048,
> +(nbd_chunk_callback) { .callback = read_cb,
> .user_data = },
When reusing the same (nbd_chunk_callback) more than once, we could
hoist it into a helper
