On Thu, Nov 24, 2016 at 09:27:51 +0800, Chen Hanxiao wrote:
> At 2016-11-24 00:19:42, "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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@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.
> >
>
> 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
