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

Michal Privoznik mprivozn at redhat.com
Tue Apr 17 15:07:41 UTC 2018


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 at redhat.com>
> 
> [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)

Michal




More information about the libvir-list mailing list