thr3ads.net - Libguestfs - Re: [Libguestfs] [PATCH nbdkit 1/2] include: Fix NBDKIT_HANDLE_NOT

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2019-Jan-14 12:15 UTC

[Libguestfs] [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.

Richard W.M. Jones

2019-Jan-14 12:15 UTC

head link

[Libguestfs] [PATCH nbdkit 1/2] include: Fix NBDKIT_HANDLE_NOT_NEEDED for C90 compilers.

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

Richard W.M. Jones

2019-Jan-14 12:15 UTC

head link

[Libguestfs] [PATCH nbdkit 2/2] tests: Test that public headers are ANSI (ISO C90) compatible.

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

Eric Blake

2019-Jan-18 14:36 UTC

head link

Re: [Libguestfs] [PATCH nbdkit 1/2] include: Fix NBDKIT_HANDLE_NOT_NEEDED for C90 compilers.

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

Eric Blake

2019-Jan-18 14:44 UTC

head link

Re: [Libguestfs] [PATCH nbdkit 2/2] tests: Test that public headers are ANSI (ISO C90) compatible.

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

Reasonably Related Threads

Search for more seemingly similar threads

Libguestfs - Jan 2019 - Re: [PATCH nbdkit 1/2] include: Fix NBDKIT_HANDLE_NOT_NEEDED for C90 compilers.

[Libguestfs] [PATCH nbdkit 0/2] tests: Test that public headers are ANSI (ISO C90) compatible.

[Libguestfs] [PATCH nbdkit 1/2] include: Fix NBDKIT_HANDLE_NOT_NEEDED for C90 compilers.

[Libguestfs] [PATCH nbdkit 2/2] tests: Test that public headers are ANSI (ISO C90) compatible.

Re: [Libguestfs] [PATCH nbdkit 1/2] include: Fix NBDKIT_HANDLE_NOT_NEEDED for C90 compilers.

Re: [Libguestfs] [PATCH nbdkit 2/2] tests: Test that public headers are ANSI (ISO C90) compatible.

Reasonably Related Threads