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

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).