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

[Chen Hanxiao]

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.

Regards,
- Chen