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@xxxxxxxxx>
> >>>
> >>> Introduce macro virCheckControllerGoto.
> >>> Jumps to a label if unsupported controller type were
> >>> passed to it.
> >>>
> >>> Signed-off-by: Chen Hanxiao <chenhanxiao@xxxxxxxxx>
> >>> ---
> >>> 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/ :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
