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

[Daniel P. Berrange]

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.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|