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@xxxxxxxxxx>
>
> [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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
