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