Eric Blake wrote:
On 08/03/2013 12:01 PM, Roman Bogorodskiy wrote:
util/vircgroup.c uses a lot of macros to detect if cgroup is supported by the system or not. These macros are pretty smart and allow to keep code compact, however the downside of that is that it's getting harder to navigate through the cgroup code.
So re-organise macros in a more simple fashion, i.e. just explicitly provide functional and stub implementation for every public function. --- src/util/vircgroup.c | 984 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 648 insertions(+), 336 deletions(-)
Doing it all at once made it harder to review. It might have been nice to break this into smaller patches (maybe convert 2-3 functions at a time, instead of all of them).
That's reasonable, I'll break patch into smaller ones.
+#if defined(__linux__) && defined(HAVE_MNTENT_H) && defined(HAVE_GETMNTENT_R) \ + && defined(_DIRENT_HAVE_D_TYPE) && defined(major) && defined(minor) +# define VIR_CGROUP_SUPPORTED
Huh - if we are requiring __linux__, then some of the other things are a given (HAVE_MNTENT_H, major, minor), while some are still dependent on having new enough kernel/glibc (_DIRENT_HAVE_D_TYPE). It might be worth trimming this down now that it is obvious we only compile the #if part on Linux; conversely, see comments in the rest of the review about conditions that you didn't factor up here yet.
+ +#if defined(VIR_CGROUP_SUPPORTED)
We prefer #ifdef VIR_CGROUP_SUPPORTED, when there is only one variable being tested.
Ok.
@@ -339,7 +332,6 @@ error: return -1; }
- static int virCgroupCopyPlacement(virCgroupPtr group,
Our style of late has been two blank lines between functions, so this change (and many others like it) is spurious.
Ok.
@@ -2609,63 +2484,6 @@ int virCgroupGetCpuacctPercpuUsage(virCgroupPtr group, char **usage)
Prior to this point, the patch is easy to follow (move the #else portions later in the file). But below here...
"cpuacct.usage_percpu", usage); }
-#ifdef _SC_CLK_TCK
I don't see _SC_CLK_TCK in the list of conditionals required by VIR_CGROUP_SUPPORTED up top; why not?
It is a separated check in the current code, so I decided to keep it separated as well.
-int virCgroupGetCpuacctStat(virCgroupPtr group, unsigned long long *user, - unsigned long long *sys) -{
...it got a bit weird, claiming that you are moving the implementation of the #if part rather than the #else part. Again, this argues why splitting the patch into more reviewable portions makes life a bit easier. Does 'git diff --patience' make the output any more legible?
-#if defined HAVE_KILL && defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
HAVE_KILL is another condition I don't see in the overriding VIR_CGROUP_SUPPORTED definition.
HAVE_KILL is also a separated check (vircgroup.c:2685), so I decided to keep the same logic. If that's always supported on Linux, I'll move it to VIR_CGROUP_SUPPORTED check.
+ +#else /* !(VIR_CGROUP_SUPPORTED) */ +bool virCgroupAvailable(void) +{ + virReportSystemError(ENXIO, "%s", + _("Control groups not supported on this platform")); + return false;
This function did NOT set an error prior to your refactoring, so it should not set an error now. When doing refactoring, you must not make any semantic changes (at least, not without documenting in the commit message that such a change was essential, but even then, separating the change from the motion is preferred). Again, an argument for splitting this into smaller, reviewable patches, by moving only 2-3 functions per patch.
Ok.
+} +#endif /* VIR_CGROUP_SUPPORTED */ + +#if defined(VIR_CGROUP_SUPPORTED) && defined(HAVE_KILL)
Why do you have to split this into a separate section? Linux has always had kill(), meaning this is effectively the same as #ifdef VIR_CGROUP_SUPPORTED.
I like where this is headed, but think it's worth another attempt.
Thanks for the review, I hope to provide updated version this weekend. Roman Bogorodskiy