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

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.