Eric Blake
2019-Aug-14 03:06 UTC
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<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> orMaybe: 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
Richard W.M. Jones
2019-Aug-14 09:21 UTC
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<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 people.redhat.com/~rjones Read my programming and virtualization blog: rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. libguestfs.org
Eric Blake
2019-Aug-14 11:44 UTC
Re: [Libguestfs] [PATCH libnbd 1/4] api: Combine callback and user_data into a single struct.
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
Seemingly Similar Threads
- Re: [PATCH libnbd 1/4] api: Combine callback and user_data into a single struct.
- Re: [PATCH libnbd 1/4] api: Combine callback and user_data into a single struct.
- [PATCH libnbd 1/4] api: Combine callback and user_data into a single struct.
- [PATCH libnbd 2/4] api: Add free function and remove valid_flag parameter.
- [PATCH libnbd] api: Rename nbd_aio_*_callback to nbd_aio_*.