On Fri, Jan 08, 2016 at 10:36:08PM +0300, Roman Bogorodskiy wrote:

From: Jasper Lievisse Adriaanse <jasper@openbsd.org>

OpenBSD uses 'struct sockpeercred' instead of 'struct ucred'. Add a configure check that detects its presence and use if in the code that could be compiled on OpenBSD.

Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- configure.ac | 7 +++++++ src/rpc/virnetsocket.c | 4 ++++ 2 files changed, 11 insertions(+)

diff --git a/configure.ac b/configure.ac index a566f5b..4a83e5c 100644 --- a/configure.ac +++ b/configure.ac @@ -393,6 +393,13 @@ AC_CHECK_TYPE([struct ifreq], #include <net/if.h> ]])

+AC_CHECK_TYPE([struct sockpeercred], + [AC_DEFINE([HAVE_STRUCT_SOCKPEERCRED], [1], + [Defined if struct sockpeercred is available])], + [], [[#include <sys/types.h> + #include <sys/socket.h>

Either sys/types.h is not needed or the build for virnetsocket.c should fail as it is not included there. It might be included transitively through some other files, but would be nice if we either specified it there as well or cleaned it up from here.

On Sun, Jan 10, 2016 at 11:29:47PM +0300, Roman Bogorodskiy wrote:

Martin Kletzander wrote:

...

On Fri, Jan 08, 2016 at 10:36:07PM +0300, Roman Bogorodskiy wrote:

...

From: Jasper Lievisse Adriaanse <jasper@openbsd.org>

Include sys/param.h along with sys/mount.h to fix compilation on OpenBSD.

Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/util/vircgroup.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index e39c4d1..e8e0875 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -27,7 +27,8 @@ #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R # include <mntent.h> #endif -#if defined HAVE_SYS_MOUNT_H +#if defined HAVE_SYS_PARAM_H && defined HAVE_SYS_MOUNT_H +# include <sys/param.h> # include <sys/mount.h>

Wouldn't this cause an error for systems that have sys/mount.h and need it for the mount() call, but don't have sys/param.h ?

That's an interesting question.

A check for sys/mount.h was added with the initial implementation of the mount() using code in commit 1da631e. The mount() call currently is used only on platforms with cgroups available (i.e. Linux).

Moreover, HAVE_SYS_MOUNT_H does not play any role in cgroups detection:

#if defined(__linux__) && defined(HAVE_GETMNTENT_R) && \ defined(_DIRENT_HAVE_D_TYPE) && defined(_SC_CLK_TCK) # define VIR_CGROUP_SUPPORTED #endif

So, sys/mount.h and mount() are only needed on Linux anyway, and I'm wondering if we actually can just do something like this:

#if defined HAVE_SYS_MOUNT_H && defined HAVE_GETMNTENT_R # include <sys/mount.h> #endif

as if we don't have HAVE_GETMNTENT_R define, we will not have VIR_CGROUP_SUPPORTED, and therefore we'll not need the mount() call.

That's true. While modifying this in my local tree I took it a little bit further in my head. First I thought why don't we include the header just above the VIR_CGROUP_SUPPORTED definition. Then I thought, hy why don't we include everything that we can there (just to get rid of future mistakes like this one with mount). And then I figured that we could split those two cgroup "implementations"

I've tested it on FreeBSD and OpenBSD and it builds fine. Unfortunately, cannot test it on Linux at this moment.

Moreove the mount() calls that are there are only used for LXC. Anyway, I tested both your approach and also including the file only around the definition of VIR_CGROUP_SUPPORTED (guarded with the HAVE_SYS_MOUNT_H of course) and it worked. But I wonder why we chose to implement cgroups in one file and not split it into two so that we don't have to deal with include issues if building without cgroups. Having said that, I don't know what's the preferred way, but now it looks like splitting into multiple files (instead of #ifdef guarding) seems better, at least to me.

Roman Bogorodskiy