Libguestfs - Aug 2020 - Re: [PATCH nbdkit 1/9] server: Add libnbdkit.so.

On 8/18/20 5:50 AM, Richard W.M. Jones wrote:
> This essentially turns the whole of nbdkit into a library.  There is a
> rump nbdkit program left which simply contains a main() function that
> forwards to the nbdkit_main() function in the library.
> 
> The reason for this is to allow nbdkit to be compiled on Windows,
> because Windows does not allow shared libraries to contain any
> undefined symbols.  nbdkit previously relied on undefined symbols in
> all plugins and filters (eg. nbdkit_parse_int) which get resolved at
> run time by the nbdkit program.  In order to make this work on Windows
> we need to have a new library (libnbdkit.so on Linux, but probably
> called something like LIBNBDKIT.DLL on Windows) which will contain
> these symbols, and plugins can then be compiled with -lnbdkit so they
> will link with this library, hence no undefined symbols.
This is a lot nicer than the last attempt we made several months ago to 
move only a bare minimum into a library.
> 
> Note this change is backwards compatible.  Because the rump nbdkit
> binary links with -lnbdkit it still contains and reexports all the
> symbols, so plugins and filters which don't explicitly link with
> -lnbdkit will still be loadable (on existing platforms).
> ---
>   server/Makefile.am                 | 21 ++++++++------
>   server/main.c                      |  2 +-
>   server/nbdkit.c                    | 46 ++++++++++++++++++++++++++++++
>   server/nbdkit.syms                 |  1 +
>   tests/test-nbdkit-backend-debug.sh | 24 ++++++++--------
>   wrapper.c                          | 10 +++++--
>   6 files changed, 80 insertions(+), 24 deletions(-)
ACK.
> -nbdkit_LDFLAGS = \
> +libnbdkit_la_LDFLAGS = \
> +	-shared $(NO_UNDEFINED_ON_WINDOWS) \
On first read, I didn't see where this was declared; but then I saw you 
have already pushed some preliminary cleanups, including adding 
NO_UNDEFINED_ON_WINDOWS in 9d052c1d.
> +++ b/server/nbdkit.c
> +#include <config.h>
> +
> +#include <stdio.h>
> +
> +extern int nbdkit_main (int argc, char *argv[]);
A bit odd to declare this in a .c; but I don't see any existing decent 
.h to put it in, nor is it worth adding a new one just for this.  So it 
is fine right here.
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  /* This does nothing except call into the real main function in
> +   * libnbdkit.so.
> +   */
> +  return nbdkit_main (argc, argv);
> +}
> diff --git a/server/nbdkit.syms b/server/nbdkit.syms
> index a67669b7..a516cc0f 100644
> --- a/server/nbdkit.syms
> +++ b/server/nbdkit.syms
> @@ -54,6 +54,7 @@
>       nbdkit_extents_new;
>       nbdkit_get_extent;
>       nbdkit_is_tls;
> +    nbdkit_main;
Do we want to export it as _nbdkit_main, to make it obvious that plugins 
shouldn't try calling it?  That's cosmetic if you think it is worth it.

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

On Tue, Aug 18, 2020 at 07:48:43AM -0500, Eric Blake
wrote:
> >+extern int nbdkit_main (int argc, char *argv[]);
> 
> A bit odd to declare this in a .c; but I don't see any existing
> decent .h to put it in, nor is it worth adding a new one just for
> this.  So it is fine right here.
Yup, better suggestions greatfully accepted, but I couldn't see
anywhere obvious.  Maybe internal.h?
> >+
> >+int
> >+main (int argc, char *argv[])
> >+{
> >+  /* This does nothing except call into the real main function in
> >+   * libnbdkit.so.
> >+   */
> >+  return nbdkit_main (argc, argv);
> >+}
> >diff --git a/server/nbdkit.syms b/server/nbdkit.syms
> >index a67669b7..a516cc0f 100644
> >--- a/server/nbdkit.syms
> >+++ b/server/nbdkit.syms
> >@@ -54,6 +54,7 @@
> >      nbdkit_extents_new;
> >      nbdkit_get_extent;
> >      nbdkit_is_tls;
> >+    nbdkit_main;
> 
> Do we want to export it as _nbdkit_main, to make it obvious that
> plugins shouldn't try calling it?  That's cosmetic if you think it
> is worth it.
I was a bit in two minds about whether this API should be public or
not.  Could it be called by other programs?  Would it be useful for
other programs?  Would that just cause us trouble in future?

I don't intend to push any of this stuff until the 1.23 development
branch opens.

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

On 8/18/20 8:53 AM, Richard W.M. Jones wrote:
> On Tue, Aug 18, 2020 at 07:48:43AM -0500, Eric Blake wrote:
>>> +extern int nbdkit_main (int argc, char *argv[]);
>>
>> A bit odd to declare this in a .c; but I don't see any existing
>> decent .h to put it in, nor is it worth adding a new one just for
>> this.  So it is fine right here.
> 
> Yup, better suggestions greatfully accepted, but I couldn't see
> anywhere obvious.  Maybe internal.h?
That would work for me.

>> Do we want to export it as _nbdkit_main, to make it obvious that
>> plugins shouldn't try calling it?  That's cosmetic if you think
it
>> is worth it.
> 
> I was a bit in two minds about whether this API should be public or
> not.  Could it be called by other programs?  Would it be useful for
> other programs?  Would that just cause us trouble in future?
I'm having a hard time seeing why any other program would want to call 
it; we assume enough control over our environment that being a library 
in the context of a larger multi-threaded app will probably violate 
those assumptions.
> 
> I don't intend to push any of this stuff until the 1.23 development
> branch opens.
Fair enough.  Then I'd better finish up my work on .list_exports, 
.default_export, and .export_description to get those done before 1.22.

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