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