Re: [libvirt] [PATCH v2 3/5] virobject: Introduce VIR_CLASS_NEW() macro

On 04/17/2018 10:32 AM, Daniel P. Berrangé wrote: > On Tue, Apr 17, 2018 at 10:20:51AM +0200, Michal Privoznik wrote: >> So far we are repeating the following lines over and over: >> >> if (!(virSomeObjectClass = virClassNew(virClassForObject(), >> "virSomeObject", >> sizeof(virSomeObject), >> virSomeObjectDispose); >> return -1; >> >> While this works, it is impossible to do some checking. Firstly, >> the class name (the 2nd argument) doesn't match the name in the >> code in all cases (the 3rd argument). Secondly, the current style >> is needlessly verbose. This commit turns example into following: >> >> VIR_CLASS_NEW(virClassForObject(), >> virSomeObject); >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > > [snip] > >> diff --git a/src/access/viraccessmanager.c b/src/access/viraccessmanager.c >> index c268ec57f7..a8d361d389 100644 >> --- a/src/access/viraccessmanager.c >> +++ b/src/access/viraccessmanager.c >> @@ -54,11 +54,8 @@ static void virAccessManagerDispose(void *obj); >> >> static int virAccessManagerOnceInit(void) >> { >> - if (!(virAccessManagerClass = virClassNew(virClassForObjectLockable(), >> - "virAccessManagerClass", >> - sizeof(virAccessManager), >> - virAccessManagerDispose))) >> - return -1; >> + VIR_CLASS_NEW(virClassForObjectLockable(), >> + virAccessManager); > > Ewww, I definitely do not like this approach - it is hiding control > flow which can exit the callpath inside a macro which is a big no. > It isn't hard to make it work in an explicit way as > > if (VIR_CLASS_NEW(virClassForObjectLockable(), > virAccessManager) < 0) > return -1; So if VIR_CLASS_NEW() should wrap virClassNew() how come this example compares the result with integer? Shouldn't hat be: if (!VIR_CLAS_NEW(..)) return -1;

or do you see VIR_CLASS_NEW defined as an expression returning integer, e.g. like this: # define VIR_CLASS_NEW(name, prnt) \ ((name##Class = virClassNew(prnt, #name, sizeof(name), name##Dispose)) ? 0 : -1)