[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] cgroup: refactor macros usage



  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: pgpDM2zirYctG.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]