[Libguestfs] [PATCH nbdkit 5/9 patch split 1/5] Create libnbdkit.so.

[Eric Blake]

On 3/26/20 3:13 PM, Richard W.M. Jones wrote:
> Currently it does nothing.

and does it well :)

> ---
>   configure.ac       |  1 +
>   Makefile.am        |  1 +
>   lib/Makefile.am    | 71 ++++++++++++++++++++++++++++++++++++++++++++++
>   server/Makefile.am |  3 ++
>   server/internal.h  |  1 +
>   lib/lib.h          | 48 +++++++++++++++++++++++++++++++
>   lib/init.c         | 53 ++++++++++++++++++++++++++++++++++
>   wrapper.c          | 18 ++++++++----
>   lib/libnbdkit.syms | 47 ++++++++++++++++++++++++++++++
>   9 files changed, 237 insertions(+), 6 deletions(-)
> 

> +++ b/lib/Makefile.am
> @@ -0,0 +1,71 @@

> +EXTRA_DIST = libnbdkit.syms
> +
> +# Note this library always has soname “libnbdkit.so.0” because plugins
> +# which may link to this library must forever have a stable soname.
> +# However the library only works with the corresponding nbdkit server
> +# binary compiled at the same time.  The two must be shipped together.
> +
> +lib_LTLIBRARIES = libnbdkit.la
> +libnbdkit_la_SOURCES = \
> +	init.c \
> +	lib.h \
> +	$(NULL)
> +
> +libnbdkit_la_CPPFLAGS = \
> +	-I$(top_srcdir)/include \
> +	-I$(top_srcdir)/common/include \
> +	-I$(top_srcdir)/common/utils \
> +	-DIN_NBDKIT_LIB=1 \
> +	$(NULL)
> +libnbdkit_la_CFLAGS = \
> +	$(PTHREAD_CFLAGS) \
> +	$(WARNINGS_CFLAGS) \
> +	$(NULL)
> +libnbdkit_la_LDFLAGS = \
> +	$(PTHREAD_LIBS) \
> +	$(NULL)
> +libnbdkit_la_LIBADD = \
> +	$(top_builddir)/common/utils/libutils.la \
> +	$(NULL)
> +
> +if USE_LINKER_SCRIPT_FOR_SERVER
> +# We have to disable the linker script for libFuzzer because Clang
> +# adds loads of fuzzer and ASAN-related symbols that are required by
> +# the plugins but which our linker script tries to hide.
> +if !ENABLE_LIBFUZZER
> +libnbdkit_la_LDFLAGS += -Wl,--version-script=$(srcdir)/libnbdkit.syms
> +endif

I don't know if you tested libfuzzer, but assume that you'll either test 
that before pushing this, or have some followup patches as needed.

> +endif

When nesting Makefile.am conditionals, I like to supply the condition on 
the endif line to make it easier to track things.  Then again, we aren't 
consistent on whether we do that in our other Makefile.am.

> diff --git a/server/Makefile.am b/server/Makefile.am
> index 4c789934..ad0de9b1 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -80,6 +80,7 @@ nbdkit_CPPFLAGS = \
>   	-Dfilterdir=\"$(filterdir)\" \
>   	-Dsbindir=\"$(sbindir)\" \
>   	-Dsysconfdir=\"$(sysconfdir)\" \
> +	-I$(top_srcdir)/lib \
>   	-I$(top_srcdir)/include \
>   	-I$(top_srcdir)/common/include \
>   	-I$(top_srcdir)/common/protocol \
> @@ -93,6 +94,7 @@ nbdkit_CFLAGS = \
>   	$(VALGRIND_CFLAGS) \
>   	$(NULL)
>   nbdkit_LDADD = \
> +	../lib/libnbdkit.la \

Is that ../ going to bite us on RHEL 7's older Automake?  Do we need to 
uses $(top_builddir) instead?

> +++ b/lib/lib.h
> @@ -0,0 +1,48 @@

> +#ifndef NBDKIT_LIB_H
> +#define NBDKIT_LIB_H
> +
> +#include <stdarg.h>
> +#include <stdbool.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +
> +/* Defines the private function which is used by the server to
> + * initialize libnbdkit.so at runtime.  This ABI may change at any
> + * time, which is why nbdkit and the corresponding libnbdkit.so must
> + * always be shipped together.
> + */
> +extern void libnbdkit_private_init (const char *expected_version);
> +

Should this read "may change at any time, other than the first parameter 
for expected_version, which is why..."?

> +#endif /* NBDKIT_LIB_H */
> diff --git a/lib/init.c b/lib/init.c

> +void
> +libnbdkit_private_init (const char *expected_version)
> +{
> +  if (strcmp (expected_version, PACKAGE_VERSION) != 0) {
> +    fprintf (stderr,
> +             "packaging error: "
> +             "nbdkit and libnbdkit.so versions do not match\n");
> +    abort ();

Works for me to keep the two in lock-step.

> +  }
> +}
> diff --git a/wrapper.c b/wrapper.c
> index 6aef81a1..eb0ba8ba 100644
> --- a/wrapper.c
> +++ b/wrapper.c

> @@ -193,7 +199,7 @@ main (int argc, char *argv[])
>     free (s);
>   
>     /* Absolute path of the real nbdkit command. */
> -  passthru_format ("%s/server/nbdkit", builddir);
> +  passthru_format ("%s/server/.libs/nbdkit", builddir);

Yep, I remember having to tweak that when playing with earlier attempts 
to fix dlopen() in VDDK.

> +++ b/lib/libnbdkit.syms

> +
> +# This linker script controls the visibility of symbols in the
> +# libnbdkit.so library.  We want to export some symbols to plugins,
> +# but at the same time we don't want plugins to be able to call
> +# arbitrary functions from nbdkit, so this script lists only the
> +# symbols we want to export.
> +
> +{
> +  # The functions we want plugins and filters to call.
> +  global:
> +
> +    # Private function that must only be called by the server.
> +    libnbdkit_private_init;
> +
> +  # Everything else is hidden.
> +  local: *;
> +};
> 

LGTM

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