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;
I'd also suggest we swap around the parameter order - have the
type name being allocated as the first method.
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
