Libguestfs - Feb 2020 - Re: [PATCH nbdkit 3/3] server: filters: Remove struct b_h.

On 2/12/20 7:40 AM, Richard W.M. Jones wrote:
> This was previously used as ‘nxdata’ and stored a tuple of ’b->next’
> and the real filter handle.  However after recent changes we don't
> need it.  We can use ‘b->next’ as nxdata, and the handle is passed to
> us by the calling functions.
> 
> Inspired by Eric Blakes observations in this email:
Blake's
> https://www.redhat.com/archives/libguestfs/2020-February/msg00092.html
> ---
>   server/filters.c | 217 ++++++++++++++++-------------------------------
>   1 file changed, 73 insertions(+), 144 deletions(-)
> 
> @@ -216,201 +205,181 @@ plugin_magic_config_key (struct backend *b)
>   static int
>   next_open (void *nxdata, int readonly)
>   {
> -  struct b_h *b_h = nxdata;
> +  struct backend *b_next = nxdata;
>   
> -  return backend_open (b_h->b, readonly);
> +  return backend_open (b_next, readonly);
>   }
With this change, 'next_open' and '(int (*)(void *,
int))backend_open'
now have identical semantics.  I'm trying to see if there are further 
changes we could make that would alleviate the need for function casts. 
I don't know if it is worth changing nbdkit-filter.h to use 'struct 
backend *' instead of 'void *' (while leaving struct backend an 
incomplete type in the public header) - it would be ABI compatible, and 
although it would require recompilation, we already state that filter 
recompilation is par for the course (since only plugins promise API 
compatibility).

But one step at a time; your patch is fine as-is.
ACK

>   /* The next_functions structure contains pointers to backend
> - * functions.  However because these functions are all expecting a
> - * backend and a handle, we cannot call them directly, but must
> - * write some next_* functions that unpack the two parameters from a
> - * single ‘void *nxdata’ struct pointer (‘b_h’).
> + * functions.  These are only needed for type safety (nxdata is void
> + * pointer, backend_* functions expect a struct backend * parameter).
> + * nxdata is a pointer to the next backend in the linked list.
>    */
>   
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Wed, Feb 12, 2020 at 07:56:38AM -0600, Eric Blake
wrote:
> On 2/12/20 7:40 AM, Richard W.M. Jones wrote:
> >This was previously used as ‘nxdata’ and stored a tuple of ’b->next’
> >and the real filter handle.  However after recent changes we don't
> >need it.  We can use ‘b->next’ as nxdata, and the handle is passed
to
> >us by the calling functions.
> >
> >Inspired by Eric Blakes observations in this email:
> 
> Blake's
> 
> >https://www.redhat.com/archives/libguestfs/2020-February/msg00092.html
> >---
> >  server/filters.c | 217
++++++++++++++++-------------------------------
> >  1 file changed, 73 insertions(+), 144 deletions(-)
> >
> 
> >@@ -216,201 +205,181 @@ plugin_magic_config_key (struct backend *b)
> >  static int
> >  next_open (void *nxdata, int readonly)
> >  {
> >-  struct b_h *b_h = nxdata;
> >+  struct backend *b_next = nxdata;
> >-  return backend_open (b_h->b, readonly);
> >+  return backend_open (b_next, readonly);
> >  }
> 
> With this change, 'next_open' and '(int (*)(void *,
> int))backend_open' now have identical semantics.  I'm trying to see
> if there are further changes we could make that would alleviate the
> need for function casts. I don't know if it is worth changing
> nbdkit-filter.h to use 'struct backend *' instead of 'void
*' (while
> leaving struct backend an incomplete type in the public header) - it
> would be ABI compatible, and although it would require
> recompilation, we already state that filter recompilation is par for
> the course (since only plugins promise API compatibility).
Yes my original version had stuff like:

static struct nbdkit_next_ops next_ops = {
  .reopen = (void *) backend_reopen,
  .get_size = (void *) backend_get_size,

but that wasn't very safe, and exporting struct backend, even
opaquely, to the public header didn't sound like a good idea either.
(Are C structs always treated the same by name?  That could cause a
problem for a filter which used "struct backend" for some internal
reason I think.)
> But one step at a time; your patch is fine as-is.
> ACK
Thanks - pushed.  Hopefully shouldn't break your new filter.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On 2/12/20 8:26 AM, Richard W.M. Jones wrote:
>> With this change, 'next_open' and '(int (*)(void *,
>> int))backend_open' now have identical semantics.  I'm trying to
see
>> if there are further changes we could make that would alleviate the
>> need for function casts. I don't know if it is worth changing
>> nbdkit-filter.h to use 'struct backend *' instead of 'void
*' (while
>> leaving struct backend an incomplete type in the public header) - it
>> would be ABI compatible, and although it would require
>> recompilation, we already state that filter recompilation is par for
>> the course (since only plugins promise API compatibility).
> 
> Yes my original version had stuff like:
> 
> static struct nbdkit_next_ops next_ops = {
>    .reopen = (void *) backend_reopen,
>    .get_size = (void *) backend_get_size,
> 
> but that wasn't very safe,
Yeah, the cast through (void*) is very imprecise, and a more precise 
cast like (int (*)(void *, int)) is a pain to write.
> and exporting struct backend, even
> opaquely, to the public header didn't sound like a good idea either.
> (Are C structs always treated the same by name?  That could cause a
> problem for a filter which used "struct backend" for some
internal
> reason I think.)
None of our in-tree filters declare 'struct backend'.  C allows two 
different .o files to declare a struct by the same name but with 
different layout, so you are right that adding an incomplete type in our 
public header COULD interfere with a pre-existing filter that already 
had an internal declaration of the same struct name.  But since we don't 
promise API stability for filters, and none of our in-tree filters would 
break, I don't see it being a problem.  I'll go ahead and propose the 
followup patch that exposes the incomplete type in the public header, 
and we can decide from there.
>> But one step at a time; your patch is fine as-is.
>> ACK
> 
> Thanks - pushed.  Hopefully shouldn't break your new filter.
Nope, no interaction, so my ext2 code is now pushed too.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org