[libvirt] [PATCH v2 0/5] Misc cleanups for internal.h & fix tests with CLang
When writing the fix for test suite mocking under CLang I found a bunch of cruft in internal.h The first four patches thus cleanup up internal.h. We then add the extra annotations requird to prevent CLang optimizer breaking mock overrides. Changed in v2: - Fixed version check to find clang - Use 'printf' instead of 'gnu_printf' on clang still - Addd fix for mock functions under clang Daniel P. Berrange (5): Remove duplicate define of __GNUC_PREREQ Require use of GCC 4.4 or CLang compilers Remove network constants out of internal.h Remove incorrectly used TODO macro Prevent more compiler optimization of mockable functions build-aux/mock-noinline.pl | 2 +- config-post.h | 24 ++--- src/check-symfile.pl | 2 +- src/internal.h | 173 ++++++++++----------------------- src/libxl/libxl_conf.c | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 1 + src/nwfilter/nwfilter_gentech_driver.c | 1 + src/qemu/qemu_capspriv.h | 2 +- src/qemu/qemu_conf.c | 1 + src/rpc/virnetsocket.h | 4 +- src/util/vircommand.h | 2 +- src/util/vircrypto.h | 2 +- src/util/virfile.h | 2 +- src/util/virhostcpu.h | 4 +- src/util/virmacaddr.h | 2 +- src/util/virnetdev.h | 8 +- src/util/virnetdevip.h | 2 +- src/util/virnetdevopenvswitch.h | 2 +- src/util/virnetdevtap.h | 6 +- src/util/virnuma.h | 16 +-- src/util/virrandom.h | 6 +- src/util/virscsi.h | 2 +- src/util/virscsivhost.h | 2 +- src/util/virsocketaddr.h | 16 +++ src/util/virtpm.h | 2 +- src/util/virutil.c | 1 + src/util/virutil.h | 10 +- src/util/viruuid.h | 2 +- src/vz/vz_sdk.c | 1 + src/xen/xen_hypervisor.c | 6 +- src/xen/xend_internal.c | 6 +- 31 files changed, 135 insertions(+), 176 deletions(-) -- 2.9.4
Back in this commit: commit b436a8ae5ccb04f8cf893d882d52ab5efc713307 Author: Fabian Freyer <fabian.freyer@physik.tu-berlin.de> Date: Thu Jun 9 00:50:35 2016 +0000 gnulib: add getopt module config-post.h was modified to define __GNUC_PREREQ, but the original definition was never removed from internal.h, and that is now dead code since config.h is always the first file included. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- config-post.h | 4 ++-- src/internal.h | 10 ---------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/config-post.h b/config-post.h index ffd0904..75e7d02 100644 --- a/config-post.h +++ b/config-post.h @@ -75,11 +75,11 @@ #endif /* LIBVIRT_NSS */ /* - * Define __GNUC__ to a sane default if it isn't yet defined. + * Define __GNUC_PREREQ to a sane default if it isn't yet defined. * This is done here so that it's included as early as possible; gnulib relies * on this to be defined in features.h, which should be included from ctype.h. * This doesn't happen on many non-glibc systems. - * When __GNUC__ is not defined, gnulib defines it to 0, which breaks things. + * When __GNUC_PREREQ is not defined, gnulib defines it to 0, which breaks things. */ #ifdef __GNUC__ # ifndef __GNUC_PREREQ diff --git a/src/internal.h b/src/internal.h index 03a973c..2ab3d48 100644 --- a/src/internal.h +++ b/src/internal.h @@ -108,16 +108,6 @@ # ifdef __GNUC__ -# ifndef __GNUC_PREREQ -# if defined __GNUC__ && defined __GNUC_MINOR__ -# define __GNUC_PREREQ(maj, min) \ - ((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min)) -# else -# define __GNUC_PREREQ(maj, min) 0 -# endif - -# endif /* __GNUC__ */ - /** * ATTRIBUTE_UNUSED: * -- 2.9.4
On Wed, Jul 05, 2017 at 12:58:47 +0100, Daniel Berrange wrote:
Back in this commit:
commit b436a8ae5ccb04f8cf893d882d52ab5efc713307 Author: Fabian Freyer <fabian.freyer@physik.tu-berlin.de> Date: Thu Jun 9 00:50:35 2016 +0000
gnulib: add getopt module
config-post.h was modified to define __GNUC_PREREQ, but the original definition was never removed from internal.h, and that is now dead code since config.h is always the first file included.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- config-post.h | 4 ++-- src/internal.h | 10 ---------- 2 files changed, 2 insertions(+), 12 deletions(-)
ACK
We only ever test libvirt with GCC or CLang which provides a GCC compatible compilation environment. Between them, these compilers cover every important operating system platform, even Windows. Mandate their use to make it explicit that we don't care about compilers like Microsoft VCC or other UNIX vendor C compilers. GCC 4.4 was picked as the baseline, since RHEL-6 ships 4.4.7 and that lets us remove a large set of checks. There is a slight issue that CLang reports itself as GCC 4.2, so we must also check if __clang__ is defined. We could check a particular CLang version too, but that would require someone to figure out a suitable min version which is fun because OS-X reports totally different CLang version numbers from CLang builds on Linux/BSD Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- config-post.h | 20 +++++----- src/internal.h | 121 ++++++++++++++++++--------------------------------------- 2 files changed, 48 insertions(+), 93 deletions(-) diff --git a/config-post.h b/config-post.h index 75e7d02..0e3b4f9 100644 --- a/config-post.h +++ b/config-post.h @@ -74,6 +74,10 @@ # undef WITH_CAPNG #endif /* LIBVIRT_NSS */ +#ifndef __GNUC__ +# error "Libvirt requires GCC >= 4.4, or CLang" +#endif + /* * Define __GNUC_PREREQ to a sane default if it isn't yet defined. * This is done here so that it's included as early as possible; gnulib relies @@ -81,13 +85,11 @@ * This doesn't happen on many non-glibc systems. * When __GNUC_PREREQ is not defined, gnulib defines it to 0, which breaks things. */ -#ifdef __GNUC__ -# ifndef __GNUC_PREREQ -# if defined __GNUC__ && defined __GNUC_MINOR__ -# define __GNUC_PREREQ(maj, min) \ - ((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min)) -# else -# define __GNUC_PREREQ(maj, min) 0 -# endif -# endif +#ifndef __GNUC_PREREQ +# define __GNUC_PREREQ(maj, min) \ + ((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min)) +#endif + +#if !(__GNUC_PREREQ(4, 4) || defined(__clang__)) +# error "Libvirt requires GCC >= 4.4, or CLang" #endif diff --git a/src/internal.h b/src/internal.h index 2ab3d48..69dff41 100644 --- a/src/internal.h +++ b/src/internal.h @@ -101,43 +101,32 @@ # define NUL_TERMINATE(buf) do { (buf)[sizeof(buf)-1] = '\0'; } while (0) # define ARRAY_CARDINALITY(Array) (sizeof(Array) / sizeof(*(Array))) -/* C99 uses __func__. __FUNCTION__ is legacy. */ -# ifndef __GNUC__ -# define __FUNCTION__ __func__ -# endif - -# ifdef __GNUC__ - /** * ATTRIBUTE_UNUSED: * * Macro to flag consciously unused parameters to functions */ -# ifndef ATTRIBUTE_UNUSED -# define ATTRIBUTE_UNUSED __attribute__((__unused__)) -# endif +# ifndef ATTRIBUTE_UNUSED +# define ATTRIBUTE_UNUSED __attribute__((__unused__)) +# endif /** * ATTRIBUTE_NORETURN: * * Macro to indicate that a function won't return to the caller */ -# ifndef ATTRIBUTE_NORETURN -# define ATTRIBUTE_NORETURN __attribute__((__noreturn__)) -# endif +# ifndef ATTRIBUTE_NORETURN +# define ATTRIBUTE_NORETURN __attribute__((__noreturn__)) +# endif /** * ATTRIBUTE_SENTINEL: * * Macro to check for NULL-terminated varargs lists */ -# ifndef ATTRIBUTE_SENTINEL -# if __GNUC_PREREQ (4, 0) -# define ATTRIBUTE_SENTINEL __attribute__((__sentinel__)) -# else -# define ATTRIBUTE_SENTINEL -# endif -# endif +# ifndef ATTRIBUTE_SENTINEL +# define ATTRIBUTE_SENTINEL __attribute__((__sentinel__)) +# endif /** * ATTRIBUTE_NOINLINE: @@ -145,13 +134,9 @@ * Force compiler not to inline a method. Should be used if * the method need to be overridable by test mocks. */ -# ifndef ATTRIBUTE_NOINLINE -# if __GNUC_PREREQ (4, 0) -# define ATTRIBUTE_NOINLINE __attribute__((__noinline__)) -# else -# define ATTRIBUTE_NOINLINE -# endif -# endif +# ifndef ATTRIBUTE_NOINLINE +# define ATTRIBUTE_NOINLINE __attribute__((__noinline__)) +# endif /** * ATTRIBUTE_FMT_PRINTF @@ -163,23 +148,19 @@ * printf format specifiers even on broken Win32 platforms * hence we have to force 'gnu_printf' for new GCC */ -# ifndef ATTRIBUTE_FMT_PRINTF -# if __GNUC_PREREQ (4, 4) -# define ATTRIBUTE_FMT_PRINTF(fmtpos, argpos) \ - __attribute__((__format__ (__gnu_printf__, fmtpos, argpos))) -# else -# define ATTRIBUTE_FMT_PRINTF(fmtpos, argpos) \ - __attribute__((__format__ (__printf__, fmtpos, argpos))) -# endif +# ifndef ATTRIBUTE_FMT_PRINTF +# ifndef __clang__ +# define ATTRIBUTE_FMT_PRINTF(fmtpos, argpos) \ + __attribute__((__format__ (__gnu_printf__, fmtpos, argpos))) +# else +# define ATTRIBUTE_FMT_PRINTF(fmtpos, argpos) \ + __attribute__((__format__ (__printf__, fmtpos, argpos))) # endif +# endif -# ifndef ATTRIBUTE_RETURN_CHECK -# if __GNUC_PREREQ (3, 4) -# define ATTRIBUTE_RETURN_CHECK __attribute__((__warn_unused_result__)) -# else -# define ATTRIBUTE_RETURN_CHECK -# endif -# endif +# ifndef ATTRIBUTE_RETURN_CHECK +# define ATTRIBUTE_RETURN_CHECK __attribute__((__warn_unused_result__)) +# endif /** * ATTRIBUTE_PACKED @@ -190,13 +171,9 @@ * ethernet packets. * Others compiler than gcc may use something different e.g. #pragma pack(1) */ -# ifndef ATTRIBUTE_PACKED -# if __GNUC_PREREQ (3, 3) -# define ATTRIBUTE_PACKED __attribute__((packed)) -# else -# error "Need an __attribute__((packed)) equivalent" -# endif -# endif +# ifndef ATTRIBUTE_PACKED +# define ATTRIBUTE_PACKED __attribute__((packed)) +# endif /* gcc's handling of attribute nonnull is less than stellar - it does * NOT improve diagnostics, and merely allows gcc to optimize away @@ -207,45 +184,21 @@ * based on whether we are compiling for real or for analysis, while * still requiring correct gcc syntax when it is turned off. See also * http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 */ -# ifndef ATTRIBUTE_NONNULL -# if __GNUC_PREREQ (3, 3) -# if STATIC_ANALYSIS -# define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m))) -# else -# define ATTRIBUTE_NONNULL(m) __attribute__(()) -# endif -# else -# define ATTRIBUTE_NONNULL(m) -# endif -# endif - -# ifndef ATTRIBUTE_FALLTHROUGH -# if __GNUC_PREREQ (7, 0) -# define ATTRIBUTE_FALLTHROUGH __attribute__((fallthrough)) -# else -# define ATTRIBUTE_FALLTHROUGH do {} while(0) -# endif +# ifndef ATTRIBUTE_NONNULL +# if STATIC_ANALYSIS +# define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m))) +# else +# define ATTRIBUTE_NONNULL(m) __attribute__(()) # endif +# endif -# else -# ifndef ATTRIBUTE_UNUSED -# define ATTRIBUTE_UNUSED -# endif -# ifndef ATTRIBUTE_FMT_PRINTF -# define ATTRIBUTE_FMT_PRINTF(...) -# endif -# ifndef ATTRIBUTE_RETURN_CHECK -# define ATTRIBUTE_RETURN_CHECK -# endif -# ifndef ATTRIBUTE_NOINLINE -# define ATTRIBUTE_NOINLINE -# endif -# -# ifndef ATTRIBUTE_FALLTHROUGH +# ifndef ATTRIBUTE_FALLTHROUGH +# if __GNUC_PREREQ (7, 0) +# define ATTRIBUTE_FALLTHROUGH __attribute__((fallthrough)) +# else # define ATTRIBUTE_FALLTHROUGH do {} while(0) # endif -# endif /* __GNUC__ */ - +# endif # if WORKING_PRAGMA_PUSH # define VIR_WARNINGS_NO_CAST_ALIGN \ -- 2.9.4
On Wed, Jul 05, 2017 at 12:58:48 +0100, Daniel Berrange wrote:
We only ever test libvirt with GCC or CLang which provides a GCC compatible compilation environment. Between them, these compilers cover every important operating system platform, even Windows.
Mandate their use to make it explicit that we don't care about compilers like Microsoft VCC or other UNIX vendor C compilers.
GCC 4.4 was picked as the baseline, since RHEL-6 ships 4.4.7 and that lets us remove a large set of checks. There is a slight issue that CLang reports itself as GCC 4.2, so we must also check if __clang__ is defined. We could check a particular CLang version too, but that would require someone to figure out a suitable min version which is fun because OS-X reports totally different CLang version numbers from CLang builds on Linux/BSD
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- config-post.h | 20 +++++----- src/internal.h | 121 ++++++++++++++++++--------------------------------------- 2 files changed, 48 insertions(+), 93 deletions(-)
ACK
The HOST_NAME_MAX, INET_ADDRSTRLEN and VIR_LOOPBACK_IPV4_ADDR constants are only used by a handful of files, so are better kept in virsocketaddr.h Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/internal.h | 16 ---------------- src/libxl/libxl_conf.c | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 1 + src/nwfilter/nwfilter_gentech_driver.c | 1 + src/qemu/qemu_conf.c | 1 + src/util/virsocketaddr.h | 16 ++++++++++++++++ src/util/virutil.c | 1 + src/vz/vz_sdk.c | 1 + 8 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/internal.h b/src/internal.h index 69dff41..2b8cc09 100644 --- a/src/internal.h +++ b/src/internal.h @@ -65,22 +65,6 @@ # include "ignore-value.h" # include "count-leading-zeros.h" -/* On architectures which lack these limits, define them (ie. Cygwin). - * Note that the libvirt code should be robust enough to handle the - * case where actual value is longer than these limits (eg. by setting - * length correctly in second argument to gethostname and by always - * using strncpy instead of strcpy). - */ -# ifndef HOST_NAME_MAX -# define HOST_NAME_MAX 256 -# endif - -# ifndef INET_ADDRSTRLEN -# define INET_ADDRSTRLEN 16 -# endif - -# define VIR_LOOPBACK_IPV4_ADDR "127.0.0.1" - /* String equality tests, suggested by Jim Meyering. */ # define STREQ(a, b) (strcmp(a, b) == 0) # define STRCASEEQ(a, b) (c_strcasecmp(a, b) == 0) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 938e09d..a85bc71 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -42,6 +42,7 @@ #include "viralloc.h" #include "viruuid.h" #include "vircommand.h" +#include "virsocketaddr.h" #include "libxl_domain.h" #include "libxl_conf.h" #include "libxl_utils.h" diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 702abe1..4436e39 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -65,6 +65,7 @@ #include "virnetdev.h" #include "virfile.h" #include "viratomic.h" +#include "virsocketaddr.h" #include "virthreadpool.h" #include "configmake.h" #include "virtime.h" diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 82e20de..3d809fb 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -37,6 +37,7 @@ #include "nwfilter_learnipaddr.h" #include "virnetdev.h" #include "datatypes.h" +#include "virsocketaddr.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_NWFILTER diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 73c33d6..a65c92a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -49,6 +49,7 @@ #include "cpu/cpu.h" #include "domain_nwfilter.h" #include "virfile.h" +#include "virsocketaddr.h" #include "virstring.h" #include "viratomic.h" #include "storage_conf.h" diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 43a3706..ae76166 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -32,6 +32,22 @@ # include <sys/un.h> # endif +/* On architectures which lack these limits, define them (ie. Cygwin). + * Note that the libvirt code should be robust enough to handle the + * case where actual value is longer than these limits (eg. by setting + * length correctly in second argument to gethostname and by always + * using strncpy instead of strcpy). + */ +# ifndef HOST_NAME_MAX +# define HOST_NAME_MAX 256 +# endif + +# ifndef INET_ADDRSTRLEN +# define INET_ADDRSTRLEN 16 +# endif + +# define VIR_LOOPBACK_IPV4_ADDR "127.0.0.1" + typedef struct { union { struct sockaddr sa; diff --git a/src/util/virutil.c b/src/util/virutil.c index e4de4ca..88fff64 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -81,6 +81,7 @@ #include "virprocess.h" #include "virstring.h" #include "virutil.h" +#include "virsocketaddr.h" verify(sizeof(gid_t) <= sizeof(unsigned int) && sizeof(uid_t) <= sizeof(unsigned int)); diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 0aa1a30..c5f11a4 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -31,6 +31,7 @@ #include "domain_conf.h" #include "virtime.h" #include "virhostcpu.h" +#include "virsocketaddr.h" #include "storage/storage_driver.h" #include "vz_sdk.h" -- 2.9.4
On Wed, Jul 05, 2017 at 12:58:49 +0100, Daniel Berrange wrote:
The HOST_NAME_MAX, INET_ADDRSTRLEN and VIR_LOOPBACK_IPV4_ADDR constants are only used by a handful of files, so are better kept in virsocketaddr.h
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/internal.h | 16 ---------------- src/libxl/libxl_conf.c | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 1 + src/nwfilter/nwfilter_gentech_driver.c | 1 + src/qemu/qemu_conf.c | 1 + src/util/virsocketaddr.h | 16 ++++++++++++++++ src/util/virutil.c | 1 + src/vz/vz_sdk.c | 1 + 8 files changed, 22 insertions(+), 16 deletions(-)
[...]
diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 43a3706..ae76166 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -32,6 +32,22 @@ # include <sys/un.h> # endif
+/* On architectures which lack these limits, define them (ie. Cygwin). + * Note that the libvirt code should be robust enough to handle the + * case where actual value is longer than these limits (eg. by setting + * length correctly in second argument to gethostname and by always + * using strncpy instead of strcpy). + */ +# ifndef HOST_NAME_MAX +# define HOST_NAME_MAX 256
I'm not entirely convinced that this semantically belongs to virsocketaddr.h At least in the virlog.c usage case it is not used in any way in regards to network communication.
+# endif + +# ifndef INET_ADDRSTRLEN +# define INET_ADDRSTRLEN 16 +# endif + +# define VIR_LOOPBACK_IPV4_ADDR "127.0.0.1" + typedef struct { union { struct sockaddr sa;
ACK to the rest though.
On Mon, Jul 10, 2017 at 01:01:26PM +0200, Peter Krempa wrote:
On Wed, Jul 05, 2017 at 12:58:49 +0100, Daniel Berrange wrote:
The HOST_NAME_MAX, INET_ADDRSTRLEN and VIR_LOOPBACK_IPV4_ADDR constants are only used by a handful of files, so are better kept in virsocketaddr.h
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/internal.h | 16 ---------------- src/libxl/libxl_conf.c | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 1 + src/nwfilter/nwfilter_gentech_driver.c | 1 + src/qemu/qemu_conf.c | 1 + src/util/virsocketaddr.h | 16 ++++++++++++++++ src/util/virutil.c | 1 + src/vz/vz_sdk.c | 1 + 8 files changed, 22 insertions(+), 16 deletions(-)
[...]
diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 43a3706..ae76166 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -32,6 +32,22 @@ # include <sys/un.h> # endif
+/* On architectures which lack these limits, define them (ie. Cygwin). + * Note that the libvirt code should be robust enough to handle the + * case where actual value is longer than these limits (eg. by setting + * length correctly in second argument to gethostname and by always + * using strncpy instead of strcpy). + */ +# ifndef HOST_NAME_MAX +# define HOST_NAME_MAX 256
I'm not entirely convinced that this semantically belongs to virsocketaddr.h
At least in the virlog.c usage case it is not used in any way in regards to network communication.
The constant is used to define a hostname string buffer, which is then used with the gethostname() system call. That's network related IMHO.
ACK to the rest though.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Jul 10, 2017 at 16:07:08 +0100, Daniel Berrange wrote:
On Mon, Jul 10, 2017 at 01:01:26PM +0200, Peter Krempa wrote:
On Wed, Jul 05, 2017 at 12:58:49 +0100, Daniel Berrange wrote:
The HOST_NAME_MAX, INET_ADDRSTRLEN and VIR_LOOPBACK_IPV4_ADDR constants are only used by a handful of files, so are better kept in virsocketaddr.h
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/internal.h | 16 ---------------- src/libxl/libxl_conf.c | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 1 + src/nwfilter/nwfilter_gentech_driver.c | 1 + src/qemu/qemu_conf.c | 1 + src/util/virsocketaddr.h | 16 ++++++++++++++++ src/util/virutil.c | 1 + src/vz/vz_sdk.c | 1 + 8 files changed, 22 insertions(+), 16 deletions(-)
[...]
diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 43a3706..ae76166 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -32,6 +32,22 @@ # include <sys/un.h> # endif
+/* On architectures which lack these limits, define them (ie. Cygwin). + * Note that the libvirt code should be robust enough to handle the + * case where actual value is longer than these limits (eg. by setting + * length correctly in second argument to gethostname and by always + * using strncpy instead of strcpy). + */ +# ifndef HOST_NAME_MAX +# define HOST_NAME_MAX 256
I'm not entirely convinced that this semantically belongs to virsocketaddr.h
At least in the virlog.c usage case it is not used in any way in regards to network communication.
The constant is used to define a hostname string buffer, which is then used with the gethostname() system call. That's network related IMHO.
I find it weird, that it gets defined in virsocketaddr.h, but the only usage is in src/util/virutil.c thus it needs cross-inclusions which were not in place prior to the move. Should perhaps virGetHostnameImpl and friends be moved too in that case?
On Tue, Jul 11, 2017 at 02:27:10PM +0200, Peter Krempa wrote:
On Mon, Jul 10, 2017 at 16:07:08 +0100, Daniel Berrange wrote:
On Mon, Jul 10, 2017 at 01:01:26PM +0200, Peter Krempa wrote:
On Wed, Jul 05, 2017 at 12:58:49 +0100, Daniel Berrange wrote:
The HOST_NAME_MAX, INET_ADDRSTRLEN and VIR_LOOPBACK_IPV4_ADDR constants are only used by a handful of files, so are better kept in virsocketaddr.h
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/internal.h | 16 ---------------- src/libxl/libxl_conf.c | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 1 + src/nwfilter/nwfilter_gentech_driver.c | 1 + src/qemu/qemu_conf.c | 1 + src/util/virsocketaddr.h | 16 ++++++++++++++++ src/util/virutil.c | 1 + src/vz/vz_sdk.c | 1 + 8 files changed, 22 insertions(+), 16 deletions(-)
[...]
diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 43a3706..ae76166 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -32,6 +32,22 @@ # include <sys/un.h> # endif
+/* On architectures which lack these limits, define them (ie. Cygwin). + * Note that the libvirt code should be robust enough to handle the + * case where actual value is longer than these limits (eg. by setting + * length correctly in second argument to gethostname and by always + * using strncpy instead of strcpy). + */ +# ifndef HOST_NAME_MAX +# define HOST_NAME_MAX 256
I'm not entirely convinced that this semantically belongs to virsocketaddr.h
At least in the virlog.c usage case it is not used in any way in regards to network communication.
The constant is used to define a hostname string buffer, which is then used with the gethostname() system call. That's network related IMHO.
I find it weird, that it gets defined in virsocketaddr.h, but the only usage is in src/util/virutil.c thus it needs cross-inclusions which were not in place prior to the move.
Should perhaps virGetHostnameImpl and friends be moved too in that case?
Lets just move HOST_NAME_MAX out of the headers entirely and just pyut it in virutil.c at the virGetHostname method definition. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Jul 11, 2017 at 13:38:56 +0100, Daniel Berrange wrote:
On Tue, Jul 11, 2017 at 02:27:10PM +0200, Peter Krempa wrote:
On Mon, Jul 10, 2017 at 16:07:08 +0100, Daniel Berrange wrote:
On Mon, Jul 10, 2017 at 01:01:26PM +0200, Peter Krempa wrote:
On Wed, Jul 05, 2017 at 12:58:49 +0100, Daniel Berrange wrote:
[...]
+# ifndef HOST_NAME_MAX +# define HOST_NAME_MAX 256
I'm not entirely convinced that this semantically belongs to virsocketaddr.h
At least in the virlog.c usage case it is not used in any way in regards to network communication.
The constant is used to define a hostname string buffer, which is then used with the gethostname() system call. That's network related IMHO.
I find it weird, that it gets defined in virsocketaddr.h, but the only usage is in src/util/virutil.c thus it needs cross-inclusions which were not in place prior to the move.
Should perhaps virGetHostnameImpl and friends be moved too in that case?
Lets just move HOST_NAME_MAX out of the headers entirely and just pyut it in virutil.c at the virGetHostname method definition.
Yep, that's reasonable. ACK to that.
The TODO macro expands to an fprintf() call and is used in several places in the Xen driver. Anything that wishes to print such debug messages should use the logging macros. In this case though, all the places in the Xen driver should have been raising a formal libvirt error instead. Add proper error handling and delete the TODO macro to prevent future misuse. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/internal.h | 9 --------- src/xen/xen_hypervisor.c | 6 ++++-- src/xen/xend_internal.c | 6 ++++-- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/internal.h b/src/internal.h index 2b8cc09..c29f20f 100644 --- a/src/internal.h +++ b/src/internal.h @@ -244,15 +244,6 @@ # define EMPTYSTR(s) ((s) ? (s) : "-") /** - * TODO: - * - * macro to flag unimplemented blocks - */ -# define TODO \ - fprintf(stderr, "Unimplemented block at %s:%d\n", \ - __FILE__, __LINE__); - -/** * SWAP: * * In place exchange of two values diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index bce7b56..e18a472 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1262,7 +1262,8 @@ xenHypervisorGetSchedulerParameters(virConnectPtr conn, } /* TODO: Implement for Xen/SEDF */ - TODO + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SEDF schedular parameters not supported")); return -1; case XEN_SCHEDULER_CREDIT: memset(&op_dom, 0, sizeof(op_dom)); @@ -1359,7 +1360,8 @@ xenHypervisorSetSchedulerParameters(virConnectPtr conn, switch (op_sys.u.getschedulerid.sched_id) { case XEN_SCHEDULER_SEDF: /* TODO: Implement for Xen/SEDF */ - TODO + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SEDF schedular parameters not supported")); return -1; case XEN_SCHEDULER_CREDIT: { memset(&op_dom, 0, sizeof(op_dom)); diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 605c3cd..6da2950 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2881,7 +2881,8 @@ xenDaemonGetSchedulerParameters(virConnectPtr conn, } /* TODO: Implement for Xen/SEDF */ - TODO + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SEDF schedular parameters not supported")); goto error; case XEN_SCHED_CRED_NPARAM: /* get cpu_weight/cpu_cap from xend/domain */ @@ -2972,7 +2973,8 @@ xenDaemonSetSchedulerParameters(virConnectPtr conn, switch (sched_nparam) { case XEN_SCHED_SEDF_NPARAM: /* TODO: Implement for Xen/SEDF */ - TODO + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SEDF schedular parameters not supported")); goto error; case XEN_SCHED_CRED_NPARAM: { char buf_weight[VIR_UUID_BUFLEN]; -- 2.9.4
On 07/05/2017 05:58 AM, Daniel P. Berrange wrote:
The TODO macro expands to an fprintf() call and is used in several places in the Xen driver. Anything that wishes to print such debug messages should use the logging macros. In this case though, all the places in the Xen driver should have been raising a formal libvirt error instead. Add proper error handling and delete the TODO macro to prevent future misuse.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/internal.h | 9 --------- src/xen/xen_hypervisor.c | 6 ++++-- src/xen/xend_internal.c | 6 ++++-- 3 files changed, 8 insertions(+), 13 deletions(-)
I responded to your V1 before seeing V2. ACK stands... Regards, Jim
diff --git a/src/internal.h b/src/internal.h index 2b8cc09..c29f20f 100644 --- a/src/internal.h +++ b/src/internal.h @@ -244,15 +244,6 @@ # define EMPTYSTR(s) ((s) ? (s) : "-")
/** - * TODO: - * - * macro to flag unimplemented blocks - */ -# define TODO \ - fprintf(stderr, "Unimplemented block at %s:%d\n", \ - __FILE__, __LINE__); - -/** * SWAP: * * In place exchange of two values diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index bce7b56..e18a472 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1262,7 +1262,8 @@ xenHypervisorGetSchedulerParameters(virConnectPtr conn, }
/* TODO: Implement for Xen/SEDF */ - TODO + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SEDF schedular parameters not supported")); return -1; case XEN_SCHEDULER_CREDIT: memset(&op_dom, 0, sizeof(op_dom)); @@ -1359,7 +1360,8 @@ xenHypervisorSetSchedulerParameters(virConnectPtr conn, switch (op_sys.u.getschedulerid.sched_id) { case XEN_SCHEDULER_SEDF: /* TODO: Implement for Xen/SEDF */ - TODO + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SEDF schedular parameters not supported")); return -1; case XEN_SCHEDULER_CREDIT: { memset(&op_dom, 0, sizeof(op_dom)); diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 605c3cd..6da2950 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2881,7 +2881,8 @@ xenDaemonGetSchedulerParameters(virConnectPtr conn, }
/* TODO: Implement for Xen/SEDF */ - TODO + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SEDF schedular parameters not supported")); goto error; case XEN_SCHED_CRED_NPARAM: /* get cpu_weight/cpu_cap from xend/domain */ @@ -2972,7 +2973,8 @@ xenDaemonSetSchedulerParameters(virConnectPtr conn, switch (sched_nparam) { case XEN_SCHED_SEDF_NPARAM: /* TODO: Implement for Xen/SEDF */ - TODO + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SEDF schedular parameters not supported")); goto error; case XEN_SCHED_CRED_NPARAM: { char buf_weight[VIR_UUID_BUFLEN];
Currently all mockable functions are annotated with the 'noinline' attribute. This is insufficient to guarantee that a function can be reliably mocked with an LD_PRELOAD. The C language spec allows the compiler to assume there is only a single implementation of each function. It can thus do things like propagating constant return values into the caller at compile time, or creating multiple specialized copies of the function body each optimized for a different caller. To prevent these optimizations we must also set the 'noclone' and 'weak' attributes. This fixes the test suite when libvirt.so is built with CLang with optimization enabled. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- build-aux/mock-noinline.pl | 2 +- src/check-symfile.pl | 2 +- src/internal.h | 23 ++++++++++++++++++----- src/qemu/qemu_capspriv.h | 2 +- src/rpc/virnetsocket.h | 4 ++-- src/util/vircommand.h | 2 +- src/util/vircrypto.h | 2 +- src/util/virfile.h | 2 +- src/util/virhostcpu.h | 4 ++-- src/util/virmacaddr.h | 2 +- src/util/virnetdev.h | 8 ++++---- src/util/virnetdevip.h | 2 +- src/util/virnetdevopenvswitch.h | 2 +- src/util/virnetdevtap.h | 6 +++--- src/util/virnuma.h | 16 ++++++++-------- src/util/virrandom.h | 6 +++--- src/util/virscsi.h | 2 +- src/util/virscsivhost.h | 2 +- src/util/virtpm.h | 2 +- src/util/virutil.h | 10 +++++----- src/util/viruuid.h | 2 +- 21 files changed, 58 insertions(+), 45 deletions(-) diff --git a/build-aux/mock-noinline.pl b/build-aux/mock-noinline.pl index eafe20d..2745d4b 100644 --- a/build-aux/mock-noinline.pl +++ b/build-aux/mock-noinline.pl @@ -43,7 +43,7 @@ sub scan_annotations { } elsif (/^\s*$/) { $func = undef; } - if (/ATTRIBUTE_NOINLINE/) { + if (/ATTRIBUTE_MOCKABLE/) { if (defined $func) { $noninlined{$func} = 1; } diff --git a/src/check-symfile.pl b/src/check-symfile.pl index d59a213..3b062d0 100755 --- a/src/check-symfile.pl +++ b/src/check-symfile.pl @@ -52,7 +52,7 @@ foreach my $elflib (@elflibs) { open NM, "-|", "nm", $elflib or die "cannot run 'nm $elflib': $!"; while (<NM>) { - next unless /^\S+\s(?:[TBD])\s(\S+)\s*$/; + next unless /^\S+\s(?:[TBDW])\s(\S+)\s*$/; $gotsyms{$1} = 1; } diff --git a/src/internal.h b/src/internal.h index c29f20f..00edd4f 100644 --- a/src/internal.h +++ b/src/internal.h @@ -113,13 +113,26 @@ # endif /** - * ATTRIBUTE_NOINLINE: + * ATTRIBUTE_MOCKABLE: + * + * Ensure that the symbol can be overridden in a mock + * library preload. This implies a number of attributes + * + * - noinline: prevents the body being inlined to + * callers, + * - noclone: prevents specialized copies of the + * function body being created for different + * callers + * - weak: prevents the compiler making optimizations + * such as constant return value propagation * - * Force compiler not to inline a method. Should be used if - * the method need to be overridable by test mocks. */ -# ifndef ATTRIBUTE_NOINLINE -# define ATTRIBUTE_NOINLINE __attribute__((__noinline__)) +# ifndef ATTRIBUTE_MOCKABLE +# if __GNUC_PREREQ(4, 5) +# define ATTRIBUTE_MOCKABLE __attribute__((__noinline__, __noclone__, __weak__)) +# else +# define ATTRIBUTE_MOCKABLE __attribute__((__noinline__, __weak__)) +# endif # endif /** diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index 94fa75b..6cc189e 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -95,7 +95,7 @@ virQEMUCapsSetCPUModelInfo(virQEMUCapsPtr qemuCaps, virCPUDefPtr virQEMUCapsProbeHostCPUForEmulator(virCapsPtr caps, virQEMUCapsPtr qemuCaps, - virDomainVirtType type) ATTRIBUTE_NOINLINE; + virDomainVirtType type) ATTRIBUTE_MOCKABLE; void virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index de795af..3c2945e 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -137,10 +137,10 @@ int virNetSocketGetUNIXIdentity(virNetSocketPtr sock, gid_t *gid, pid_t *pid, unsigned long long *timestamp) - ATTRIBUTE_NOINLINE; + ATTRIBUTE_MOCKABLE; int virNetSocketGetSELinuxContext(virNetSocketPtr sock, char **context) - ATTRIBUTE_NOINLINE; + ATTRIBUTE_MOCKABLE; int virNetSocketSetBlocking(virNetSocketPtr sock, bool blocking); diff --git a/src/util/vircommand.h b/src/util/vircommand.h index e7c2e51..c042a53 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -58,7 +58,7 @@ enum { void virCommandPassFD(virCommandPtr cmd, int fd, - unsigned int flags) ATTRIBUTE_NOINLINE; + unsigned int flags) ATTRIBUTE_MOCKABLE; void virCommandPassListenFDs(virCommandPtr cmd); diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h index 068602f..50400c6 100644 --- a/src/util/vircrypto.h +++ b/src/util/vircrypto.h @@ -55,6 +55,6 @@ int virCryptoEncryptData(virCryptoCipher algorithm, ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9) ATTRIBUTE_RETURN_CHECK; -uint8_t *virCryptoGenerateRandom(size_t nbytes) ATTRIBUTE_NOINLINE; +uint8_t *virCryptoGenerateRandom(size_t nbytes) ATTRIBUTE_MOCKABLE; #endif /* __VIR_CRYPTO_H__ */ diff --git a/src/util/virfile.h b/src/util/virfile.h index 57ceb80..32c9115 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -188,7 +188,7 @@ void virFileActivateDirOverride(const char *argv0) off_t virFileLength(const char *path, int fd) ATTRIBUTE_NONNULL(1); bool virFileIsDir (const char *file) ATTRIBUTE_NONNULL(1); -bool virFileExists(const char *file) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NOINLINE; +bool virFileExists(const char *file) ATTRIBUTE_NONNULL(1) ATTRIBUTE_MOCKABLE; bool virFileIsExecutable(const char *file) ATTRIBUTE_NONNULL(1); enum { diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h index 67033de..3b30a0d 100644 --- a/src/util/virhostcpu.h +++ b/src/util/virhostcpu.h @@ -38,7 +38,7 @@ bool virHostCPUHasBitmap(void); virBitmapPtr virHostCPUGetPresentBitmap(void); virBitmapPtr virHostCPUGetOnlineBitmap(void); int virHostCPUGetCount(void); -int virHostCPUGetThreadsPerSubcore(virArch arch) ATTRIBUTE_NOINLINE; +int virHostCPUGetThreadsPerSubcore(virArch arch) ATTRIBUTE_MOCKABLE; int virHostCPUGetMap(unsigned char **cpumap, unsigned int *online, @@ -51,7 +51,7 @@ int virHostCPUGetInfo(virArch hostarch, unsigned int *cores, unsigned int *threads); -int virHostCPUGetKVMMaxVCPUs(void) ATTRIBUTE_NOINLINE; +int virHostCPUGetKVMMaxVCPUs(void) ATTRIBUTE_MOCKABLE; int virHostCPUStatsAssign(virNodeCPUStatsPtr param, const char *name, diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h index f4f5e2c..79492cd 100644 --- a/src/util/virmacaddr.h +++ b/src/util/virmacaddr.h @@ -48,7 +48,7 @@ void virMacAddrGetRaw(const virMacAddr *src, unsigned char dst[VIR_MAC_BUFLEN]); const char *virMacAddrFormat(const virMacAddr *addr, char *str); void virMacAddrGenerate(const unsigned char prefix[VIR_MAC_PREFIX_BUFLEN], - virMacAddrPtr addr) ATTRIBUTE_NOINLINE; + virMacAddrPtr addr) ATTRIBUTE_MOCKABLE; int virMacAddrParse(const char* str, virMacAddrPtr addr) ATTRIBUTE_RETURN_CHECK; int virMacAddrParseHex(const char* str, diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 51fcae5..2e9a9c4 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -156,7 +156,7 @@ int virNetDevExists(const char *brname) int virNetDevSetOnline(const char *ifname, bool online) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE; + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE; int virNetDevGetOnline(const char *ifname, bool *online) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; @@ -164,7 +164,7 @@ int virNetDevGetOnline(const char *ifname, int virNetDevSetMAC(const char *ifname, const virMacAddr *macaddr) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE; + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE; int virNetDevGetMAC(const char *ifname, virMacAddrPtr macaddr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; @@ -303,8 +303,8 @@ int virNetDevSysfsFile(char **pf_sysfs_device_link, const char *ifname, const char *file) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE; + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE; int virNetDevRunEthernetScript(const char *ifname, const char *script) - ATTRIBUTE_NOINLINE; + ATTRIBUTE_MOCKABLE; #endif /* __VIR_NETDEV_H__ */ diff --git a/src/util/virnetdevip.h b/src/util/virnetdevip.h index 6b509ea..972a49a 100644 --- a/src/util/virnetdevip.h +++ b/src/util/virnetdevip.h @@ -67,7 +67,7 @@ int virNetDevIPAddrAdd(const char *ifname, virSocketAddr *addr, virSocketAddr *peer, unsigned int prefix) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE; + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE; int virNetDevIPRouteAdd(const char *ifname, virSocketAddrPtr addr, unsigned int prefix, diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 51bb1dd..dc677ca 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -59,6 +59,6 @@ int virNetDevOpenvswitchInterfaceStats(const char *ifname, int virNetDevOpenvswitchGetVhostuserIfname(const char *path, char **ifname) - ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE; + ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE; #endif /* __VIR_NETDEV_OPENVSWITCH_H__ */ diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h index 0b17feb..1c4343e 100644 --- a/src/util/virnetdevtap.h +++ b/src/util/virnetdevtap.h @@ -39,7 +39,7 @@ int virNetDevTapCreate(char **ifname, int *tapfd, size_t tapfdSize, unsigned int flags) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE; + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE; int virNetDevTapDelete(const char *ifname, const char *tunpath) @@ -49,7 +49,7 @@ int virNetDevTapGetName(int tapfd, char **ifname) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; char* virNetDevTapGetRealDeviceName(char *ifname) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE; + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE; typedef enum { VIR_NETDEV_TAP_CREATE_NONE = 0, @@ -89,7 +89,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, unsigned int *actualMTU, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE; + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE; int virNetDevTapInterfaceStats(const char *ifname, virDomainInterfaceStatsPtr stats) diff --git a/src/util/virnuma.h b/src/util/virnuma.h index e4e1fd0..62b89e9 100644 --- a/src/util/virnuma.h +++ b/src/util/virnuma.h @@ -34,20 +34,20 @@ int virNumaSetupMemoryPolicy(virDomainNumatuneMemMode mode, virBitmapPtr nodeset); virBitmapPtr virNumaGetHostMemoryNodeset(void); -bool virNumaNodesetIsAvailable(virBitmapPtr nodeset) ATTRIBUTE_NOINLINE; -bool virNumaIsAvailable(void) ATTRIBUTE_NOINLINE; -int virNumaGetMaxNode(void) ATTRIBUTE_NOINLINE; -bool virNumaNodeIsAvailable(int node) ATTRIBUTE_NOINLINE; +bool virNumaNodesetIsAvailable(virBitmapPtr nodeset) ATTRIBUTE_MOCKABLE; +bool virNumaIsAvailable(void) ATTRIBUTE_MOCKABLE; +int virNumaGetMaxNode(void) ATTRIBUTE_MOCKABLE; +bool virNumaNodeIsAvailable(int node) ATTRIBUTE_MOCKABLE; int virNumaGetDistances(int node, int **distances, - int *ndistances) ATTRIBUTE_NOINLINE; + int *ndistances) ATTRIBUTE_MOCKABLE; int virNumaGetNodeMemory(int node, unsigned long long *memsize, - unsigned long long *memfree) ATTRIBUTE_NOINLINE; + unsigned long long *memfree) ATTRIBUTE_MOCKABLE; unsigned int virNumaGetMaxCPUs(void); -int virNumaGetNodeCPUs(int node, virBitmapPtr *cpus) ATTRIBUTE_NOINLINE; +int virNumaGetNodeCPUs(int node, virBitmapPtr *cpus) ATTRIBUTE_MOCKABLE; int virNumaGetPageInfo(int node, unsigned int page_size, @@ -59,7 +59,7 @@ int virNumaGetPages(int node, unsigned int **pages_avail, unsigned int **pages_free, size_t *npages) - ATTRIBUTE_NONNULL(5) ATTRIBUTE_NOINLINE; + ATTRIBUTE_NONNULL(5) ATTRIBUTE_MOCKABLE; int virNumaSetPagePoolSize(int node, unsigned int page_size, unsigned long long page_count, diff --git a/src/util/virrandom.h b/src/util/virrandom.h index 7a984ee..990a456 100644 --- a/src/util/virrandom.h +++ b/src/util/virrandom.h @@ -24,11 +24,11 @@ # include "internal.h" -uint64_t virRandomBits(int nbits) ATTRIBUTE_NOINLINE; +uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE; double virRandom(void); uint32_t virRandomInt(uint32_t max); int virRandomBytes(unsigned char *buf, size_t buflen) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE; -int virRandomGenerateWWN(char **wwn, const char *virt_type) ATTRIBUTE_NOINLINE; + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE; +int virRandomGenerateWWN(char **wwn, const char *virt_type) ATTRIBUTE_MOCKABLE; #endif /* __VIR_RANDOM_H__ */ diff --git a/src/util/virscsi.h b/src/util/virscsi.h index 9f8b3ec..eed563d 100644 --- a/src/util/virscsi.h +++ b/src/util/virscsi.h @@ -37,7 +37,7 @@ char *virSCSIDeviceGetSgName(const char *sysfs_prefix, const char *adapter, unsigned int bus, unsigned int target, - unsigned long long unit) ATTRIBUTE_NOINLINE; + unsigned long long unit) ATTRIBUTE_MOCKABLE; char *virSCSIDeviceGetDevName(const char *sysfs_prefix, const char *adapter, unsigned int bus, diff --git a/src/util/virscsivhost.h b/src/util/virscsivhost.h index 21887dd..f9272a6 100644 --- a/src/util/virscsivhost.h +++ b/src/util/virscsivhost.h @@ -61,6 +61,6 @@ void virSCSIVHostDeviceGetUsedBy(virSCSIVHostDevicePtr dev, const char **drv_name, const char **dom_name); void virSCSIVHostDeviceFree(virSCSIVHostDevicePtr dev); -int virSCSIVHostOpenVhostSCSI(int *vhostfd) ATTRIBUTE_NOINLINE; +int virSCSIVHostOpenVhostSCSI(int *vhostfd) ATTRIBUTE_MOCKABLE; #endif /* __VIR_SCSIHOST_H__ */ diff --git a/src/util/virtpm.h b/src/util/virtpm.h index b21fc05..7067bb5 100644 --- a/src/util/virtpm.h +++ b/src/util/virtpm.h @@ -22,6 +22,6 @@ #ifndef __VIR_TPM_H__ # define __VIR_TPM_H__ -char *virTPMCreateCancelPath(const char *devpath) ATTRIBUTE_NOINLINE; +char *virTPMCreateCancelPath(const char *devpath) ATTRIBUTE_MOCKABLE; #endif /* __VIR_TPM_H__ */ diff --git a/src/util/virutil.h b/src/util/virutil.h index 4938255..35e5ca3 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -139,8 +139,8 @@ char *virGetUserConfigDirectory(void); char *virGetUserCacheDirectory(void); char *virGetUserRuntimeDirectory(void); char *virGetUserShell(uid_t uid); -char *virGetUserName(uid_t uid) ATTRIBUTE_NOINLINE; -char *virGetGroupName(gid_t gid) ATTRIBUTE_NOINLINE; +char *virGetUserName(uid_t uid) ATTRIBUTE_MOCKABLE; +char *virGetGroupName(gid_t gid) ATTRIBUTE_MOCKABLE; int virGetGroupList(uid_t uid, gid_t group, gid_t **groups) ATTRIBUTE_NONNULL(3); int virGetUserID(const char *name, @@ -201,12 +201,12 @@ verify((int)VIR_TRISTATE_BOOL_ABSENT == (int)VIR_TRISTATE_SWITCH_ABSENT); unsigned int virGetListenFDs(void); -long virGetSystemPageSize(void) ATTRIBUTE_NOINLINE; -long virGetSystemPageSizeKB(void) ATTRIBUTE_NOINLINE; +long virGetSystemPageSize(void) ATTRIBUTE_MOCKABLE; +long virGetSystemPageSizeKB(void) ATTRIBUTE_MOCKABLE; unsigned long long virMemoryLimitTruncate(unsigned long long value); bool virMemoryLimitIsSet(unsigned long long value); -unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_NOINLINE; +unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_MOCKABLE; /** * VIR_ASSIGN_IS_OVERFLOW: diff --git a/src/util/viruuid.h b/src/util/viruuid.h index 1d67e9e..3b41b42 100644 --- a/src/util/viruuid.h +++ b/src/util/viruuid.h @@ -49,7 +49,7 @@ int virGetHostUUID(unsigned char *host_uuid) ATTRIBUTE_NONNULL(1); int virUUIDIsValid(unsigned char *uuid); -int virUUIDGenerate(unsigned char *uuid) ATTRIBUTE_NOINLINE; +int virUUIDGenerate(unsigned char *uuid) ATTRIBUTE_MOCKABLE; int virUUIDParse(const char *uuidstr, unsigned char *uuid) -- 2.9.4
On Wed, Jul 05, 2017 at 12:58:51 +0100, Daniel Berrange wrote:
Currently all mockable functions are annotated with the 'noinline' attribute. This is insufficient to guarantee that a function can be reliably mocked with an LD_PRELOAD. The C language spec allows the compiler to assume there is only a single implementation of each function. It can thus do things like propagating constant return values into the caller at compile time, or creating multiple specialized copies of the function body each optimized for a different caller. To prevent these optimizations we must also set the 'noclone' and 'weak' attributes.
This fixes the test suite when libvirt.so is built with CLang with optimization enabled.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- build-aux/mock-noinline.pl | 2 +- src/check-symfile.pl | 2 +- src/internal.h | 23 ++++++++++++++++++----- src/qemu/qemu_capspriv.h | 2 +- src/rpc/virnetsocket.h | 4 ++-- src/util/vircommand.h | 2 +- src/util/vircrypto.h | 2 +- src/util/virfile.h | 2 +- src/util/virhostcpu.h | 4 ++-- src/util/virmacaddr.h | 2 +- src/util/virnetdev.h | 8 ++++---- src/util/virnetdevip.h | 2 +- src/util/virnetdevopenvswitch.h | 2 +- src/util/virnetdevtap.h | 6 +++--- src/util/virnuma.h | 16 ++++++++-------- src/util/virrandom.h | 6 +++--- src/util/virscsi.h | 2 +- src/util/virscsivhost.h | 2 +- src/util/virtpm.h | 2 +- src/util/virutil.h | 10 +++++----- src/util/viruuid.h | 2 +- 21 files changed, 58 insertions(+), 45 deletions(-)
ACK
Hi On Mon, Jul 10, 2017 at 1:14 PM Peter Krempa <pkrempa@redhat.com> wrote:
On Wed, Jul 05, 2017 at 12:58:51 +0100, Daniel Berrange wrote:
Currently all mockable functions are annotated with the 'noinline' attribute. This is insufficient to guarantee that a function can be reliably mocked with an LD_PRELOAD. The C language spec allows the compiler to assume there is only a single implementation of each function. It can thus do things like propagating constant return values into the caller at compile time, or creating multiple specialized copies of the function body each optimized for a different caller. To prevent these optimizations we must also set the 'noclone' and 'weak' attributes.
This fixes the test suite when libvirt.so is built with CLang with optimization enabled.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
This patch makes virtlogd crash: (gdb) bt #0 0x0000000000000000 in ?? () #1 0x00005555555a8084 in virHashCreateFull (size=size@entry=5, dataFree=0x5555555b0930 <virObjectFreeHashData>, keyCode=keyCode@entry=0x5555555a7c30 <virHashStrCode>, keyEqual=keyEqual@entry=0x5555555a7c10 <virHashStrEqual>, keyCopy=keyCopy@entry=0x5555555a7bb0 <virHashStrCopy>, keyFree=keyFree@entry=0x5555555a7b90 <virHashStrFree>) at util/virhash.c:167 #2 0x00005555555a8151 in virHashCreate (size=size@entry=5, dataFree=<optimized out>) at util/virhash.c:196 #3 0x00005555555779f0 in virNetDaemonNew () at rpc/virnetdaemon.c:137 #4 0x00005555555708ec in virLogDaemonNew (privileged=false, config=0x555555820940) at logging/log_daemon.c:163 #5 main (argc=<optimized out>, argv=0x7fffffffd888) at logging/log_daemon.c:1069 any idea?
---
build-aux/mock-noinline.pl | 2 +- src/check-symfile.pl | 2 +- src/internal.h | 23 ++++++++++++++++++----- src/qemu/qemu_capspriv.h | 2 +- src/rpc/virnetsocket.h | 4 ++-- src/util/vircommand.h | 2 +- src/util/vircrypto.h | 2 +- src/util/virfile.h | 2 +- src/util/virhostcpu.h | 4 ++-- src/util/virmacaddr.h | 2 +- src/util/virnetdev.h | 8 ++++---- src/util/virnetdevip.h | 2 +- src/util/virnetdevopenvswitch.h | 2 +- src/util/virnetdevtap.h | 6 +++--- src/util/virnuma.h | 16 ++++++++-------- src/util/virrandom.h | 6 +++--- src/util/virscsi.h | 2 +- src/util/virscsivhost.h | 2 +- src/util/virtpm.h | 2 +- src/util/virutil.h | 10 +++++----- src/util/viruuid.h | 2 +- 21 files changed, 58 insertions(+), 45 deletions(-)
ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
On Wed, Jul 12, 2017 at 1:54 AM Marc-André Lureau < marcandre.lureau@gmail.com> wrote:
Hi
On Mon, Jul 10, 2017 at 1:14 PM Peter Krempa <pkrempa@redhat.com> wrote:
On Wed, Jul 05, 2017 at 12:58:51 +0100, Daniel Berrange wrote:
Currently all mockable functions are annotated with the 'noinline' attribute. This is insufficient to guarantee that a function can be reliably mocked with an LD_PRELOAD. The C language spec allows the compiler to assume there is only a single implementation of each function. It can thus do things like propagating constant return values into the caller at compile time, or creating multiple specialized copies of the function body each optimized for a different caller. To prevent these optimizations we must also set the 'noclone' and 'weak' attributes.
This fixes the test suite when libvirt.so is built with CLang with optimization enabled.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
This patch makes virtlogd crash:
(gdb) bt #0 0x0000000000000000 in ?? () #1 0x00005555555a8084 in virHashCreateFull (size=size@entry=5, dataFree=0x5555555b0930 <virObjectFreeHashData>, keyCode=keyCode@entry=0x5555555a7c30 <virHashStrCode>, keyEqual=keyEqual@entry=0x5555555a7c10 <virHashStrEqual>, keyCopy=keyCopy@entry=0x5555555a7bb0 <virHashStrCopy>, keyFree=keyFree@entry=0x5555555a7b90 <virHashStrFree>) at util/virhash.c:167 #2 0x00005555555a8151 in virHashCreate (size=size@entry=5, dataFree=<optimized out>) at util/virhash.c:196 #3 0x00005555555779f0 in virNetDaemonNew () at rpc/virnetdaemon.c:137 #4 0x00005555555708ec in virLogDaemonNew (privileged=false, config=0x555555820940) at logging/log_daemon.c:163 #5 main (argc=<optimized out>, argv=0x7fffffffd888) at logging/log_daemon.c:1069
any idea?
(btw, this is on f25, gcc (GCC) 6.3.1 20161221 (Red Hat 6.3.1-1))
---
build-aux/mock-noinline.pl | 2 +- src/check-symfile.pl | 2 +- src/internal.h | 23 ++++++++++++++++++----- src/qemu/qemu_capspriv.h | 2 +- src/rpc/virnetsocket.h | 4 ++-- src/util/vircommand.h | 2 +- src/util/vircrypto.h | 2 +- src/util/virfile.h | 2 +- src/util/virhostcpu.h | 4 ++-- src/util/virmacaddr.h | 2 +- src/util/virnetdev.h | 8 ++++---- src/util/virnetdevip.h | 2 +- src/util/virnetdevopenvswitch.h | 2 +- src/util/virnetdevtap.h | 6 +++--- src/util/virnuma.h | 16 ++++++++-------- src/util/virrandom.h | 6 +++--- src/util/virscsi.h | 2 +- src/util/virscsivhost.h | 2 +- src/util/virtpm.h | 2 +- src/util/virutil.h | 10 +++++----- src/util/viruuid.h | 2 +- 21 files changed, 58 insertions(+), 45 deletions(-)
ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
-- Marc-André Lureau
On Tue, Jul 11, 2017 at 11:54:21PM +0000, Marc-André Lureau wrote:
Hi
On Mon, Jul 10, 2017 at 1:14 PM Peter Krempa <pkrempa@redhat.com> wrote:
On Wed, Jul 05, 2017 at 12:58:51 +0100, Daniel Berrange wrote:
Currently all mockable functions are annotated with the 'noinline' attribute. This is insufficient to guarantee that a function can be reliably mocked with an LD_PRELOAD. The C language spec allows the compiler to assume there is only a single implementation of each function. It can thus do things like propagating constant return values into the caller at compile time, or creating multiple specialized copies of the function body each optimized for a different caller. To prevent these optimizations we must also set the 'noclone' and 'weak' attributes.
This fixes the test suite when libvirt.so is built with CLang with optimization enabled.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
This patch makes virtlogd crash:
(gdb) bt #0 0x0000000000000000 in ?? () #1 0x00005555555a8084 in virHashCreateFull (size=size@entry=5, dataFree=0x5555555b0930 <virObjectFreeHashData>, keyCode=keyCode@entry=0x5555555a7c30 <virHashStrCode>, keyEqual=keyEqual@entry=0x5555555a7c10 <virHashStrEqual>, keyCopy=keyCopy@entry=0x5555555a7bb0 <virHashStrCopy>, keyFree=keyFree@entry=0x5555555a7b90 <virHashStrFree>) at util/virhash.c:167 #2 0x00005555555a8151 in virHashCreate (size=size@entry=5, dataFree=<optimized out>) at util/virhash.c:196 #3 0x00005555555779f0 in virNetDaemonNew () at rpc/virnetdaemon.c:137 #4 0x00005555555708ec in virLogDaemonNew (privileged=false, config=0x555555820940) at logging/log_daemon.c:163 #5 main (argc=<optimized out>, argv=0x7fffffffd888) at logging/log_daemon.c:1069
any idea?
I can reproduce that too, so i'll investigate Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrange wrote:
When writing the fix for test suite mocking under CLang I found a bunch of cruft in internal.h The first four patches thus cleanup up internal.h. We then add the extra annotations requird to prevent CLang optimizer breaking mock overrides.
Changed in v2:
- Fixed version check to find clang - Use 'printf' instead of 'gnu_printf' on clang still - Addd fix for mock functions under clang
Daniel P. Berrange (5): Remove duplicate define of __GNUC_PREREQ Require use of GCC 4.4 or CLang compilers Remove network constants out of internal.h Remove incorrectly used TODO macro Prevent more compiler optimization of mockable functions
build-aux/mock-noinline.pl | 2 +- config-post.h | 24 ++--- src/check-symfile.pl | 2 +- src/internal.h | 173 ++++++++++----------------------- src/libxl/libxl_conf.c | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 1 + src/nwfilter/nwfilter_gentech_driver.c | 1 + src/qemu/qemu_capspriv.h | 2 +- src/qemu/qemu_conf.c | 1 + src/rpc/virnetsocket.h | 4 +- src/util/vircommand.h | 2 +- src/util/vircrypto.h | 2 +- src/util/virfile.h | 2 +- src/util/virhostcpu.h | 4 +- src/util/virmacaddr.h | 2 +- src/util/virnetdev.h | 8 +- src/util/virnetdevip.h | 2 +- src/util/virnetdevopenvswitch.h | 2 +- src/util/virnetdevtap.h | 6 +- src/util/virnuma.h | 16 +-- src/util/virrandom.h | 6 +- src/util/virscsi.h | 2 +- src/util/virscsivhost.h | 2 +- src/util/virsocketaddr.h | 16 +++ src/util/virtpm.h | 2 +- src/util/virutil.c | 1 + src/util/virutil.h | 10 +- src/util/viruuid.h | 2 +- src/vz/vz_sdk.c | 1 + src/xen/xen_hypervisor.c | 6 +- src/xen/xend_internal.c | 6 +- 31 files changed, 135 insertions(+), 176 deletions(-)
Works fine for me with clang 3.9.1 and clang 4.0.0. Roman Bogorodskiy
On Wed, 2017-07-05 at 12:58 +0100, Daniel P. Berrange wrote:
When writing the fix for test suite mocking under CLang I found a bunch of cruft in internal.h The first four patches thus cleanup up internal.h. We then add the extra annotations requird to prevent CLang optimizer breaking mock overrides.
A very minor detail that I feel compelled to point out regardless: it's Clang, not CLang. Please use the proper capitalization, if not in the commit messages at the very least in the error messages you introduce in patch 2/5. -- Andrea Bolognani / Red Hat / Virtualization
participants (6)
-
Andrea Bolognani -
Daniel P. Berrange -
Jim Fehlig -
Marc-André Lureau -
Peter Krempa -
Roman Bogorodskiy