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/