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

[Eric Blake]

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

-------------- 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/20190814/82a43ab7/attachment.sig>