[libvirt] [PATCH 1/2] cgroup: fix including sys/mount.h for OpenBSD

[Roman Bogorodskiy]

  Martin Kletzander wrote:

> >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.

AFAIR I touched this code maybe 2 years ago or so to make stuff easier
to maintain for platforms without cgroups support and I remember also
Eric Blake helped a lot with that.

I don't remember if it was discussed to have a separate file for stubs
though.

Anyway, it seems that the header issue is the only inconvenience caused
by the current approach. Otherwise it feels OK to be able to add new
functions and the corresponding stubs in scope of a single file.
Considering that it's very unlikely that there will be some other real
implementation but Linux one, probably it's fine as it is (though I
don't touch cgroups related code often, so my opinion could be biased).

Roman Bogorodskiy