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.
Regards,
- Chen
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
