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

Daniel P. Berrangé berrange at redhat.com
Tue Apr 17 15:10:43 UTC 2018


On Tue, Apr 17, 2018 at 05:07:41PM +0200, Michal Privoznik wrote:
> 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;

Yes, my bad - I had  VIR_ALLOC() on the brain when i mistakenly
wrote < 0 instead of == NULL (or  just !).

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

No, it was a mistake.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|




More information about the libvir-list mailing list