On Thu, Nov 24, 2016 at 09:27:51 +0800, Chen Hanxiao wrote:
At 2016-11-24 00:19:42, "Daniel P. Berrange" <berrange@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@gmail.com>
Introduce macro virCheckControllerGoto. Jumps to a label if unsupported controller type were passed to it.
Signed-off-by: Chen Hanxiao <chenhanxiao@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.
I agree with Dan here. We should not make the code look less C-like. I originally did not want to rant about this but my preference is not to do stuff like this. I concur with Dan's NACK