[libvirt] [PATCH 1/4] internal: introduce macro virCheckControllerGoto

[Peter Krempa]

On Thu, Nov 24, 2016 at 09:27:51 +0800, Chen Hanxiao wrote:
> At 2016-11-24 00:19:42, "Daniel P. Berrange" <berrange at redhat.com> wrote:
> >On Wed, Nov 23, 2016 at 11:17:13AM -0500, John Ferlan wrote:
> >> On 11/23/2016 10:55 AM, Daniel P. Berrange wrote:
> >> > On Wed, Nov 23, 2016 at 10:49:00AM -0500, John Ferlan wrote:
> >> >> On 11/03/2016 11:09 PM, Chen Hanxiao wrote:
> >> >>> From: Chen Hanxiao <chenhanxiao at gmail.com>
> >> >>>
> >> >>> Introduce macro virCheckControllerGoto.
> >> >>> Jumps to a label if unsupported controller type were
> >> >>> passed to it.
> >> >>>
> >> >>> Signed-off-by: Chen Hanxiao <chenhanxiao at gmail.com>
> >> >>> ---
> >> >>>  src/internal.h | 19 +++++++++++++++++++
> >> >>>  1 file changed, 19 insertions(+)
> >> >>>
> >> >>> diff --git a/src/internal.h b/src/internal.h
> >> >>> index d8cc5ad..a34f43e 100644
> >> >>> --- a/src/internal.h
> >> >>> +++ b/src/internal.h
> >> >>> @@ -362,6 +362,25 @@
> >> >>>          }                                                               \
> >> >>>      } while (0)
> >> >>>  
> >> >>> +/**
> >> >>> + * virCheckControllerGoto:
> >> >>> + * @Cgp: virCgroupPtr pointer
> >> >>> + * @CgpCtr: cgroup controller type enum
> >> >>> + * @label: label to jump to on error
> >> >>> + *
> >> >>> + * Returns nothing. Jumps to a label if unsupported controller type were
> >> >>> + * passed to it.
> >> >>> + */
> >> >>> +# define virCheckControllerGoto(Cgp, CgpCtr, label)                    \
> >> >>> +    do {                                                               \
> >> >>> +        if (!virCgroupHasController(Cgp, CgpCtr)) {                    \
> >> >>> +            virReportError(VIR_ERR_OPERATION_INVALID,                  \
> >> >>> +                            _("cgroup %s controller is not mounted"),  \
> >> >>                               ^
> >> >> Extra space
> >> >>
> >> >> s/%s/'%s'/
> >> >>
> >> >> ACK (I'll adjust before pushing)
> >> > 
> >> > I'm personally *not* in favour of doing this. I don't think that macros
> >> > should be used to hide control flow logic, especially when they're hiding
> >> > 'goto'.
> >> > 
> >> > Now, we do have some of this style macros for checking flags. I think those
> >> > are more acceptable because flag checking is always done right at the
> >> > start of the function.
> >> > 
> >> > These cgroup macros by contrast will be placed at arbitrary locations
> >> > in the middle of functions, hiding the fact that we're jumping right
> >> > out.
> >> > 
> >> > So personally I'm NACK on this series, unless there contrasting opinions
> >> > people want to put forward.
> >> > 
> >> 
> >> I'm ambivalent - to a degree I saw this is as "similar to" places where
> >> we create capitalized macro names and use the "goto" to jump control. In
> >> most of those cases, the label *isn't* included in the macro as a
> >> parameter (use cscope and egroup search on "goto.*\\").
> >> 
> >> Since "Goto" is in the name, it felt "reasonable" especially since the
> >> label is included as a parameter. If label wasn't a parameter, then my
> >> opinion would have been different, especially seeing as how in certain
> >> functions we will change between 'cleanup', 'error', and/or 'endjob'
> >> depending on where we are in the function.  So even though cleanup could
> >> exist, if the code moved inside a job going to cleanup wouldn't be the
> >> right thing (of course this logic also gives credence to your position
> >> for not making this change).
> >
> >For me it just doesn't fit with the way I scan code. When I'm looking
> >at the control flow/structure  of a method I'm not closely reading the
> >method names to see if they contain the suffix "Goto" - I'm just scanning
> >the structure for normal C constructs for/if/while/goto.
> >
> 
> We had a lot of similar code for doing such kind of thing, such as:
> 
> virCheckControllerGoto.
> 
> There are many other xxxGoto in internal.h.
> 
> We had a lot of similar cgroup controller check works, 
> So a glance at the macros and its name may not bring too much troubles.
> The macro in this set did help like other similar macros.

I agree with Dan here. We should not make the code look less C-like. I
originally did not want to rant about this but my preference is not to
do stuff like this.

I concur with Dan's NACK
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161124/b36d1fce/attachment-0001.sig>