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

At 2016-11-24 00:19:42, "Daniel P. Berrange" <berrange(a)redhat.com&gt; 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(a)gmail.com&gt; >> >>> >> >>> Introduce macro virCheckControllerGoto. >> >>> Jumps to a label if unsupported controller type were >> >>> passed to it. >> >>> >> >>> Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com&gt; >> >>> --- >> >>> 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.