[PATCH nbdkit 0/2] tests: Test that public headers are ANSI (ISO C90) compatible.
We previously discussed allowing the plugin API to be consumed by non-GCC/non-Clang/old compilers. This implements a test. Rich.
When an ANSI/C90 plugin compiled with ‘-pedantic’ uses NBDKIT_HANDLE_NOT_NEEDED it gets the error: ISO C forbids conversion of function pointer to object pointer type This is because the existing macro worked by returning a function pointer but in C90 function pointers cannot be cast to data pointers since on some ancient architectures code and data pointers were incompatible. We only need a convenient global data pointer here, and the address of ‘errno’ should be fine. --- include/nbdkit-common.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h index 36fce20..cb9954e 100644 --- a/include/nbdkit-common.h +++ b/include/nbdkit-common.h @@ -40,6 +40,7 @@ #include <stdarg.h> #include <stdint.h> +#include <errno.h> #ifdef __cplusplus extern "C" { @@ -78,7 +79,7 @@ extern char *nbdkit_realpath (const char *path); /* A static non-NULL pointer which can be used when you don't need a * per-connection handle. */ -#define NBDKIT_HANDLE_NOT_NEEDED ((void *) &nbdkit_error) +#define NBDKIT_HANDLE_NOT_NEEDED (&errno) #ifdef __cplusplus } -- 2.20.1
On 1/14/19 6:15 AM, Richard W.M. Jones wrote:
When an ANSI/C90 plugin compiled with ‘-pedantic’ uses NBDKIT_HANDLE_NOT_NEEDED it gets the error:
ISO C forbids conversion of function pointer to object pointer type
While POSIX requires it to work. But such is life.
This is because the existing macro worked by returning a function pointer but in C90 function pointers cannot be cast to data pointers since on some ancient architectures code and data pointers were incompatible.
We only need a convenient global data pointer here, and the address of ‘errno’ should be fine.
errno is often implemented as a dereference of a function call in order to get thread-safe semantics; for example, your patch produces '&(*__errno_location())' on glibc. That should be okay (especially since it silenced the warning). Another option, instead of referencing an actual variable, could be writing ((void*)(intptr_t)1). Either way, this patch makes sense.
@@ -78,7 +79,7 @@ extern char *nbdkit_realpath (const char *path); /* A static non-NULL pointer which can be used when you don't need a * per-connection handle. */ -#define NBDKIT_HANDLE_NOT_NEEDED ((void *) &nbdkit_error) +#define NBDKIT_HANDLE_NOT_NEEDED (&errno)
#ifdef __cplusplus }
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Jan 18, 2019 at 08:36:07AM -0600, Eric Blake wrote:
On 1/14/19 6:15 AM, Richard W.M. Jones wrote:
When an ANSI/C90 plugin compiled with ‘-pedantic’ uses NBDKIT_HANDLE_NOT_NEEDED it gets the error:
ISO C forbids conversion of function pointer to object pointer type
While POSIX requires it to work. But such is life.
This is because the existing macro worked by returning a function pointer but in C90 function pointers cannot be cast to data pointers since on some ancient architectures code and data pointers were incompatible.
We only need a convenient global data pointer here, and the address of ‘errno’ should be fine.
errno is often implemented as a dereference of a function call in order to get thread-safe semantics; for example, your patch produces '&(*__errno_location())' on glibc. That should be okay (especially since it silenced the warning).
Yeah, I was aware of this and looking for some other global variable but couldn't think of one at the time. Hmm, how about optind? Rich.
Another option, instead of referencing an actual variable, could be writing ((void*)(intptr_t)1).
Either way, this patch makes sense.
@@ -78,7 +79,7 @@ extern char *nbdkit_realpath (const char *path); /* A static non-NULL pointer which can be used when you don't need a * per-connection handle. */ -#define NBDKIT_HANDLE_NOT_NEEDED ((void *) &nbdkit_error) +#define NBDKIT_HANDLE_NOT_NEEDED (&errno)
#ifdef __cplusplus }
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On 1/18/19 8:47 AM, Richard W.M. Jones wrote:
On Fri, Jan 18, 2019 at 08:36:07AM -0600, Eric Blake wrote:
On 1/14/19 6:15 AM, Richard W.M. Jones wrote:
When an ANSI/C90 plugin compiled with ‘-pedantic’ uses NBDKIT_HANDLE_NOT_NEEDED it gets the error:
ISO C forbids conversion of function pointer to object pointer type
While POSIX requires it to work. But such is life.
This is because the existing macro worked by returning a function pointer but in C90 function pointers cannot be cast to data pointers since on some ancient architectures code and data pointers were incompatible.
We only need a convenient global data pointer here, and the address of ‘errno’ should be fine.
errno is often implemented as a dereference of a function call in order to get thread-safe semantics; for example, your patch produces '&(*__errno_location())' on glibc. That should be okay (especially since it silenced the warning).
Yeah, I was aware of this and looking for some other global variable but couldn't think of one at the time.
Hmm, how about optind?
<getopt.h> is not portable, and I'm not sure if <unistd.h> universally declares optind. gnulib states that MSVC 14 lacks <unistd.h>. Other POSIX standardized global variables: environ (but no standardized header declares it) daylight, getdate_err, timezone, tzname (<time.h>, but both marked XSI so not necessarily on all platforms) optarg, opterr, optopt (<unistd.h>, same boat as optind) signgam (<math.h>, but marked XSI, and requires -lm on some platforms) stdin, stdout, stderr (<stdio.h>) Also, pulling in the address of one of these forces the variable to be linked in. For errno or stdin, that's okay (you're probably linking it in anyways), but for the others, it may (slightly) bloat the resulting binary, by referencing an otherwise unused variable.
Rich.
Another option, instead of referencing an actual variable, could be writing ((void*)(intptr_t)1).
Either way, this patch makes sense.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Jan 18, 2019 at 09:08:27AM -0600, Eric Blake wrote:
errno is often implemented as a dereference of a function call in order to get thread-safe semantics; for example, your patch produces '&(*__errno_location())' on glibc. That should be okay (especially since it silenced the warning).
Come to think of it, I got mixed up about your point. This isn't a problem because __errno_location() returns a pointer to an integer which is then dereferenced, so the fact there is a function is irrelevant. Thanks for the other analysis about available variables. I'll stick to errno. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
In commit 3775916120749e935d287f35cb29ff6c586656df (and before that in discussions with Eric) we decided that we should try to allow plugins to be compiled with C compilers other than GCC or Clang (or with very old / peculiar / incompatible versions of those compilers). However until we test this we cannot guarantee that changes to the code will not break this in future, hence this test. Note that GCC or Clang is still required to compile nbdkit itself. --- tests/test-ansi-c-plugin.c | 167 +++++++++++++++++++++++++++++++++++++ tests/Makefile.am | 24 ++++++ tests/test-ansi-c.sh | 39 +++++++++ 3 files changed, 230 insertions(+) diff --git a/tests/test-ansi-c-plugin.c b/tests/test-ansi-c-plugin.c new file mode 100644 index 0000000..8596ca5 --- /dev/null +++ b/tests/test-ansi-c-plugin.c @@ -0,0 +1,167 @@ +/* nbdkit + * Copyright (C) 2013-2019 Red Hat Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include <nbdkit-plugin.h> + +/* This looks like a 100MB disk with one empty partition. */ +static unsigned char bootsector[512] = { + 0xfa, 0xb8, 0x00, 0x10, 0x8e, 0xd0, 0xbc, 0x00, + 0xb0, 0xb8, 0x00, 0x00, 0x8e, 0xd8, 0x8e, 0xc0, + 0xfb, 0xbe, 0x00, 0x7c, 0xbf, 0x00, 0x06, 0xb9, + 0x00, 0x02, 0xf3, 0xa4, 0xea, 0x21, 0x06, 0x00, + 0x00, 0xbe, 0xbe, 0x07, 0x38, 0x04, 0x75, 0x0b, + 0x83, 0xc6, 0x10, 0x81, 0xfe, 0xfe, 0x07, 0x75, + 0xf3, 0xeb, 0x16, 0xb4, 0x02, 0xb0, 0x01, 0xbb, + 0x00, 0x7c, 0xb2, 0x80, 0x8a, 0x74, 0x01, 0x8b, + 0x4c, 0x02, 0xcd, 0x13, 0xea, 0x00, 0x7c, 0x00, + 0x00, 0xeb, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0xe1, 0x5b, 0x02, 0x00, 0x00, 0x00, 0x00, 0x02, + 0x03, 0x00, 0x83, 0xbc, 0x31, 0x0c, 0x80, 0x00, + 0x00, 0x00, 0x01, 0x1f, 0x03, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x55, 0xaa +}; +static unsigned char data[104857600]; + +static void +ansi_c_load (void) +{ + memcpy (data, bootsector, sizeof bootsector); +} + +static void * +ansi_c_open (int readonly) +{ + return NBDKIT_HANDLE_NOT_NEEDED; +} + +static int64_t +ansi_c_get_size (void *handle) +{ + return (int64_t) sizeof (data); +} + +#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS + +static int +ansi_c_pread (void *handle, void *buf, uint32_t count, uint64_t offset) +{ + memcpy (buf, data+offset, count); + return 0; +} + +/* Strictly speaking (and we're compiling with -pedantic, so we _are_ + * strictly speaking), ANSI C did not allow struct initialization + * using labels. However it was a common extension to C compilers of + * the period. + * + * Therefore we rely here on the order of fields in struct + * nbdkit_plugin. But that's OK because of nbdkit's stable ABI + * guarantee. + * + * If you are really writing an nbdkit plugin which can only use C90 + * then you are advised to find the extension to your compiler which + * allows you to initialize named fields. + */ +static struct nbdkit_plugin plugin = { + 0, 0, 0, + "ansic", + 0, + PACKAGE_VERSION, + 0, + ansi_c_load, + 0, + 0, 0, 0, + ansi_c_open, + 0, + ansi_c_get_size, + 0, 0, 0, 0, + ansi_c_pread +}; + +NBDKIT_REGISTER_PLUGIN(plugin) diff --git a/tests/Makefile.am b/tests/Makefile.am index 823b6c5..e96dac9 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -40,6 +40,7 @@ EXTRA_DIST = \ shebang.pl \ shebang.py \ shebang.rb \ + test-ansi-c.sh \ test-blocksize.sh \ test-cache.sh \ test-cache-max-size.sh \ @@ -161,6 +162,29 @@ test_socket_activation_CFLAGS = $(WARNINGS_CFLAGS) endif HAVE_PLUGINS +# This builds a plugin using an ANSI (ISO C90) compiler to ensure that +# the header file is compatible. The plugin does nothing very +# interesting, it's mainly a compile test. +TESTS += \ + test-ansi-c.sh +# check_LTLIBRARIES won't build a shared library (see automake manual). +# So we have to do this and add a dependency. +noinst_LTLIBRARIES += test-ansi-c-plugin.la +test-ansi-c.sh: test-ansi-c-plugin.la + +test_ansi_c_plugin_la_SOURCES = \ + test-ansi-c-plugin.c \ + $(top_srcdir)/include/nbdkit-plugin.h +test_ansi_c_plugin_la_CPPFLAGS = \ + -std=c90 -pedantic \ + -I$(top_srcdir)/include +test_ansi_c_plugin_la_CFLAGS = \ + $(WARNINGS_CFLAGS) +# For use of the -rpath option, see: +# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html +test_ansi_c_plugin_la_LDFLAGS = \ + -module -avoid-version -shared -rpath /nowhere + if HAVE_CXX # This builds a plugin and a filter using the C++ compiler. They # don't do anything interesting when run. diff --git a/tests/test-ansi-c.sh b/tests/test-ansi-c.sh new file mode 100755 index 0000000..5942fad --- /dev/null +++ b/tests/test-ansi-c.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2017-2019 Red Hat Inc. +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +set -e + +# We only really care that the plugin compiled, so we don't +# need to run it properly. +libs=./.libs +nbdkit $libs/test-ansi-c-plugin.so --version -- 2.20.1
On 1/14/19 6:15 AM, Richard W.M. Jones wrote:
In commit 3775916120749e935d287f35cb29ff6c586656df (and before that in discussions with Eric) we decided that we should try to allow plugins to be compiled with C compilers other than GCC or Clang (or with very old / peculiar / incompatible versions of those compilers).
However until we test this we cannot guarantee that changes to the code will not break this in future, hence this test.
Note that GCC or Clang is still required to compile nbdkit itself. --- tests/test-ansi-c-plugin.c | 167 +++++++++++++++++++++++++++++++++++++ tests/Makefile.am | 24 ++++++ tests/test-ansi-c.sh | 39 +++++++++ 3 files changed, 230 insertions(+)
diff --git a/tests/test-ansi-c-plugin.c b/tests/test-ansi-c-plugin.c new file mode 100644 index 0000000..8596ca5 --- /dev/null +++ b/tests/test-ansi-c-plugin.c
+ +#include <config.h>
Do we really want to be including <config.h> in this file? This is the one case where we know we are standalone; I'm also worried that the macros defined in config.h were determined based on the existence of extensions during configure probes, and may be inaccurately-defined for compilation under a different set of compiler flags. I'd omit this (but perhaps leave a comment why), as proof that we are truly independent of how nbdkit was configured.
+/* Strictly speaking (and we're compiling with -pedantic, so we _are_
lol
+ * strictly speaking), ANSI C did not allow struct initialization + * using labels. However it was a common extension to C compilers of + * the period. + * + * Therefore we rely here on the order of fields in struct + * nbdkit_plugin. But that's OK because of nbdkit's stable ABI + * guarantee. + * + * If you are really writing an nbdkit plugin which can only use C90 + * then you are advised to find the extension to your compiler which + * allows you to initialize named fields. + */ +static struct nbdkit_plugin plugin = { + 0, 0, 0,
I'd typically use NULL for pointers, but using 0 is strictly portable, so I'm fine with it.
+ "ansic", + 0, + PACKAGE_VERSION, + 0, + ansi_c_load, + 0, + 0, 0, 0, + ansi_c_open, + 0, + ansi_c_get_size, + 0, 0, 0, 0, + ansi_c_pread +}; + +NBDKIT_REGISTER_PLUGIN(plugin)
+++ b/tests/Makefile.am
+# This builds a plugin using an ANSI (ISO C90) compiler to ensure that +# the header file is compatible. The plugin does nothing very +# interesting, it's mainly a compile test. +TESTS += \ + test-ansi-c.sh +# check_LTLIBRARIES won't build a shared library (see automake manual). +# So we have to do this and add a dependency. +noinst_LTLIBRARIES += test-ansi-c-plugin.la +test-ansi-c.sh: test-ansi-c-plugin.la + +test_ansi_c_plugin_la_SOURCES = \ + test-ansi-c-plugin.c \ + $(top_srcdir)/include/nbdkit-plugin.h +test_ansi_c_plugin_la_CPPFLAGS = \ + -std=c90 -pedantic \ + -I$(top_srcdir)/include +test_ansi_c_plugin_la_CFLAGS = \ + $(WARNINGS_CFLAGS) +# For use of the -rpath option, see: +# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html +test_ansi_c_plugin_la_LDFLAGS = \ + -module -avoid-version -shared -rpath /nowhere
Looks sane, if it still works without <config.h>. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Jan 18, 2019 at 08:44:44AM -0600, Eric Blake wrote:
On 1/14/19 6:15 AM, Richard W.M. Jones wrote:
In commit 3775916120749e935d287f35cb29ff6c586656df (and before that in discussions with Eric) we decided that we should try to allow plugins to be compiled with C compilers other than GCC or Clang (or with very old / peculiar / incompatible versions of those compilers).
However until we test this we cannot guarantee that changes to the code will not break this in future, hence this test.
Note that GCC or Clang is still required to compile nbdkit itself. --- tests/test-ansi-c-plugin.c | 167 +++++++++++++++++++++++++++++++++++++ tests/Makefile.am | 24 ++++++ tests/test-ansi-c.sh | 39 +++++++++ 3 files changed, 230 insertions(+)
diff --git a/tests/test-ansi-c-plugin.c b/tests/test-ansi-c-plugin.c new file mode 100644 index 0000000..8596ca5 --- /dev/null +++ b/tests/test-ansi-c-plugin.c
+ +#include <config.h>
Do we really want to be including <config.h> in this file? This is the one case where we know we are standalone; I'm also worried that the macros defined in config.h were determined based on the existence of extensions during configure probes, and may be inaccurately-defined for compilation under a different set of compiler flags. I'd omit this (but perhaps leave a comment why), as proof that we are truly independent of how nbdkit was configured.
No we shouldn't have it - this was a bug. I removed it and it turned out it _was_ being used for to define PACKAGE_VERSION in the plugin struct, but I'll replace that. And fix the NULL / 0 pointer business too. Thanks, Rich.
+/* Strictly speaking (and we're compiling with -pedantic, so we _are_
lol
+ * strictly speaking), ANSI C did not allow struct initialization + * using labels. However it was a common extension to C compilers of + * the period. + * + * Therefore we rely here on the order of fields in struct + * nbdkit_plugin. But that's OK because of nbdkit's stable ABI + * guarantee. + * + * If you are really writing an nbdkit plugin which can only use C90 + * then you are advised to find the extension to your compiler which + * allows you to initialize named fields. + */ +static struct nbdkit_plugin plugin = { + 0, 0, 0,
I'd typically use NULL for pointers, but using 0 is strictly portable, so I'm fine with it.
+ "ansic", + 0, + PACKAGE_VERSION, + 0, + ansi_c_load, + 0, + 0, 0, 0, + ansi_c_open, + 0, + ansi_c_get_size, + 0, 0, 0, 0, + ansi_c_pread +}; + +NBDKIT_REGISTER_PLUGIN(plugin)
+++ b/tests/Makefile.am
+# This builds a plugin using an ANSI (ISO C90) compiler to ensure that +# the header file is compatible. The plugin does nothing very +# interesting, it's mainly a compile test. +TESTS += \ + test-ansi-c.sh +# check_LTLIBRARIES won't build a shared library (see automake manual). +# So we have to do this and add a dependency. +noinst_LTLIBRARIES += test-ansi-c-plugin.la +test-ansi-c.sh: test-ansi-c-plugin.la + +test_ansi_c_plugin_la_SOURCES = \ + test-ansi-c-plugin.c \ + $(top_srcdir)/include/nbdkit-plugin.h +test_ansi_c_plugin_la_CPPFLAGS = \ + -std=c90 -pedantic \ + -I$(top_srcdir)/include +test_ansi_c_plugin_la_CFLAGS = \ + $(WARNINGS_CFLAGS) +# For use of the -rpath option, see: +# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html +test_ansi_c_plugin_la_LDFLAGS = \ + -module -avoid-version -shared -rpath /nowhere
Looks sane, if it still works without <config.h>.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
-- 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
participants (2)
-
Eric Blake -
Richard W.M. Jones