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
Attachment:
pgp58XNuy6FoY.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list