[Libguestfs] [PATCH nbdkit 6/9] tests: Add a regression test that we can still compile with -undefined.

[Richard W.M. Jones]

On Thu, Mar 26, 2020 at 02:05:11PM -0500, Eric Blake wrote:
> On 3/26/20 1:25 PM, Richard W.M. Jones wrote:
> >Note that this probably isn't testing anything because at least on
> >Linux the -undefined/-no-undefined flags appear to have no effect on
> >linking.
> >
> 
> [skipping review of 5/9 for now, due to its size.  I don't know how
> easy or hard it would be to split it into bisectable pieces: first
> introducing the library with just the parsing symbols that can be
> done locally, then adding more symbols, before finally getting to
> where -no-undefined actually works for plugins. But as you said on
> IRC, it gets tricky to have to get a half-finished library to still
> work correctly, so it may be worth leaving that one squashed]

I'll see if there is anything easy I can do here.  It may be that the
tests still work because libtool doesn't really care about undefined etc.

> >I'm using the -undefined flag here, but this is not actually a flag
> >documented anywhere in the libtool documentation, but I assume it's
> >fine because libtool doesn't complain.
> 
> I'll have to research if it is just ignoring an unknown flag, or
> actually honoring it (but even if it is ignoring the flag, libtool's
> default behavior of allowing undefined symbols on Linux is what we
> are indeed trying to test).  This test will have to be conditional
> on platform (it won't work on mingw); so you may have to revive the
> configure.ac stuff you chopped out in patch 4.

OK.

> >---
> >  tests/Makefile.am             | 28 ++++++++++++++
> >  tests/test-undefined.sh       | 36 ++++++++++++++++++
> >  tests/test-undefined-plugin.c | 70 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 134 insertions(+)
> >
> >diff --git a/tests/Makefile.am b/tests/Makefile.am
> >index f9c6b8ad..901dca7e 100644
> >--- a/tests/Makefile.am
> 
> >+# Build a plugin with libtool -undefined flag.  This is how plugins
> >+# were built before libnbdkit.so existed.
> >+TESTS += \
> >+	test-undefined.sh \
> >+	$(NULL)
> >
> 
> >+# For use of the -rpath option, see:
> >+# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html
> >+test_undefined_plugin_la_LDFLAGS = \
> >+	-module -avoid-version -shared \
> >+	-undefined \
> >+	$(SHARED_LDFLAGS) \
> >+	-rpath /nowhere \
> >+	$(NULL)
> 
> The most important part here is that we did NOT pass in libnbdkit.so
> on the link line, but it still builds, resulting in a plugin with
> undefined symbols which can still be loaded by the just-built
> nbdkit.
> 
> >+++ b/tests/test-undefined-plugin.c
> >@@ -0,0 +1,70 @@
> 
> >+
> >+#include <config.h>
> 
> Does our plugin need <config.h>?  Or can we omit that to be more
> like a true out-of-tree plugin that did not opt-in to our library?

Actually yes, for PACKAGE_VERSION.  However that isn't fundamentally
required.

> >+#include <stdio.h>
> >+#include <stdlib.h>
> >+#include <string.h>
> >+
> >+#include <nbdkit-plugin.h>
> >+
> >+static void *
> >+undefined_open (int readonly)
> >+{
> >+  return NBDKIT_HANDLE_NOT_NEEDED;
> 
> Hmm. I don't see any _use_ of an nbdkit_* function here.  It's not
> enough that this library links without libnbdkit.so, but we need to
> be testing that when it uses an undefined symbol, such a symbol
> actually gets provided via the nbdkit binary dlopen()ing this
> plugin.

Hmm that is actually a very good point, back to the drawing board on
this one!  I can add some calls to nbdkit_debug or as you suggested
nbdkit_parse_*.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/