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).
> +#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.
> @@ -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.
> @@ -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?
> -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.
> +
> +#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.
> +}
> +#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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list