Re: [PATCH 00/17] conf/qemu/etc: change several functions to return void instead of 0/-1
On Wed, Feb 26, 2025 at 11:04:23AM +0100, Martin Kletzander wrote:
On Tue, Feb 11, 2025 at 11:28:53PM -0500, Laine Stump wrote:
> This started with me noticing one function call that checked for
> failure on something that I knew couldn't fail. So I changed that
> function to return void. But then I noticed another similar function
> that also should return void, so I changed that one too. Then
> eliminating the check for the return from those functions caused
> another function to become infallible, so I changed that one too,
> which led to more and more until the evening was finished and I had
> this long list of tiny mechanical patches.
The classic Hal fixing a lightbulb moment ;)
> And here it is - an easy way to improve your review stats :-)
>
Well, it definitely did not look like it in the list =D
> P.S. I know there are more of these, but forced myself to stop here.
>
Good for you! If you have something in the back of your mind, then feel
free to mention it in gitlab issue labelled bitesizedtask, we still
mention them and try to give them to newcomers or people who seem
interested in starting out.
> A related question - is it possible for virObjectNew() to fail? I did
> finally find (after some searching, documentation that says
> g_object_new() can't return null, but I don't know enough about
> g_object stuff to know if the vir*Initialize functions could fail (for
> example). If virObjectNew() can't fail then that open a whole new can
> of worms...
>
It can't... almost, from my point of view. The allocation parts should
abort the whole process, but if you want to create a new object of a
class which is not of type object, then it will return NULL. That
"should not happen", but... you already know how the cookie crumbles.
Having said that, we might just abort ourselves and make our own
guarantee that virObjectNew() does *not* return NULL, ever.
virObjectNew effectively can't fail - glib would return NULL if
klass->type is invalid, but that would be impossible because
we can't get an initialized 'klass' without a valid 'type' field.
IOW, the only way g_object_new would return NULL is upon memory
corruption of 'klass'. That's not something we need to check
ourselves, memory corruption is just a standard C feature you
get to enjoy periodically ;-)
The problem with all these virXXXNew() functions though is not
the virObjectNew call, it is the virClassNew call. These are
usually put inside a VIR_ONCE_GLOBAL_INIT function that the
must be called before using virObjectNew. These global init
functions can fail because virOnce() can fail, because
pthread_once() can fail, or because the init callback can
fail due to virClassNew failing.
With some refactoring we can probably eliminate this. For
virClassNew the two failure scenarios are:
if (parent == NULL &&
STRNEQ(name, "virObject")) {
virReportInvalidNonNullArg(parent);
return NULL;
} else if (objectSize <= parentSize ||
parentSize != (parent ? parent->objectSize : 0)) {
virReportInvalidArg(objectSize,
_("object size %1$zu of %2$s is not larger than parent
class %3$zu"),
objectSize, name, parent->objectSize);
return NULL;
}
All callers actually use the VIR_CLASS_NEW macro instead. If
we could get a static assert into VIR_CLASS_NEW to enforce
the object size checks, then that assert would likely prevent
'parent == NULL' too, because we would need the parent to be
a valid type name, not NULL. IOW, we can likely make
VIR_CLASS_NEW impossible to fail at runtime.
That leaves the virOnce() failure problem.
For that we could possibly copy QEMU and switch to using
__constructor__ functions instead of virOnce. Those are
thus called on our behalf and we no longer have a failure
scenario around the pthread_once() call.
At that point all our virXXXNew() functions would be impossible
to fail.
With 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 :|