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/