On Wed, Feb 26, 2025 at 10:23:12AM +0000, Daniel P. Berrangé wrote:
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.
I cannot recall, but I think there were some reasons why we did not want to use library constructors in libvirt. But really can't put my finger on it, it's like the reason is over 10 years old, so maybe it's not relevant any more...
At that point all our virXXXNew() functions would be impossible to fail.
It would be nice to have more "infallible" functions, though.
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 :|
Attachment:
signature.asc
Description: PGP signature
