src/bhyve/bhyve_domain.c | 13 ++- src/conf/domain_addr.c | 6 +- src/conf/domain_conf.c | 174 ++++++++++----------------------- src/conf/domain_conf.h | 16 +-- src/hyperv/hyperv_driver.c | 28 ++---- src/libxl/libxl_conf.c | 4 +- src/libxl/libxl_domain.c | 11 +-- src/libxl/libxl_driver.c | 11 +-- src/libxl/xen_common.c | 15 +-- src/libxl/xen_common.h | 2 +- src/libxl/xen_xl.c | 4 +- src/lxc/lxc_driver.c | 6 +- src/qemu/qemu_domain_address.c | 8 +- src/qemu/qemu_driver.c | 14 +-- src/qemu/qemu_postparse.c | 58 ++++------- src/qemu/qemu_process.c | 3 +- src/vbox/vbox_common.c | 13 +-- src/vmx/vmx.c | 12 +-- src/vz/vz_driver.c | 11 +-- src/vz/vz_sdk.c | 14 +-- 20 files changed, 126 insertions(+), 297 deletions(-)
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. And here it is - an easy
way to improve your review stats :-)
P.S. I know there are more of these, but forced myself to stop here.
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...
Laine Stump (17):
conf: change virDomainHostdevInsert() to return void
conf: change virDomainNetInsert() to return void
conf: change virDomainFSInsert() to return void
conf: change virDomainShmemDefInsert() to return void
conf: change virDomainDefMaybeAddInput() to return void
libxl: change xenDomainDefAddImplicitInputDevice() to return void
conf: change qemuDomainDefAddImplicitInputDevice() to return void
conf: stop checking for NULL return from virDomainControllerDefNew()
conf: stop checking for NULL return from virDomainDefAddController()
conf: change virDomainDefAddUSBController() to return void
hyperv: change hypervDomainDefAppendController() to return void
conf: change virDomainDefMaybeAddController() to return true/false
conf: change virDomainDefMaybeAddHostdevSCSIcontroller() to return
void
conf: change virDomainDefAddDiskControllersForType() to return void
conf: change virDomainDefMaybeAddVirtioSerialController() to return
void
conf: change virDomainDefMaybeAddSmartcardController() to return void
conf: change virDomainDefAddImplicitControllers() to return void
src/bhyve/bhyve_domain.c | 13 ++-
src/conf/domain_addr.c | 6 +-
src/conf/domain_conf.c | 174 ++++++++++-----------------------
src/conf/domain_conf.h | 16 +--
src/hyperv/hyperv_driver.c | 28 ++----
src/libxl/libxl_conf.c | 4 +-
src/libxl/libxl_domain.c | 11 +--
src/libxl/libxl_driver.c | 11 +--
src/libxl/xen_common.c | 15 +--
src/libxl/xen_common.h | 2 +-
src/libxl/xen_xl.c | 4 +-
src/lxc/lxc_driver.c | 6 +-
src/qemu/qemu_domain_address.c | 8 +-
src/qemu/qemu_driver.c | 14 +--
src/qemu/qemu_postparse.c | 58 ++++-------
src/qemu/qemu_process.c | 3 +-
src/vbox/vbox_common.c | 13 +--
src/vmx/vmx.c | 12 +--
src/vz/vz_driver.c | 11 +--
src/vz/vz_sdk.c | 14 +--
20 files changed, 126 insertions(+), 297 deletions(-)
--
2.47.1
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. >Laine Stump (17): > conf: change virDomainHostdevInsert() to return void > conf: change virDomainNetInsert() to return void > conf: change virDomainFSInsert() to return void > conf: change virDomainShmemDefInsert() to return void > conf: change virDomainDefMaybeAddInput() to return void > libxl: change xenDomainDefAddImplicitInputDevice() to return void > conf: change qemuDomainDefAddImplicitInputDevice() to return void > conf: stop checking for NULL return from virDomainControllerDefNew() > conf: stop checking for NULL return from virDomainDefAddController() > conf: change virDomainDefAddUSBController() to return void > hyperv: change hypervDomainDefAppendController() to return void > conf: change virDomainDefMaybeAddController() to return true/false > conf: change virDomainDefMaybeAddHostdevSCSIcontroller() to return > void > conf: change virDomainDefAddDiskControllersForType() to return void > conf: change virDomainDefMaybeAddVirtioSerialController() to return > void > conf: change virDomainDefMaybeAddSmartcardController() to return void > conf: change virDomainDefAddImplicitControllers() to return void > I'll have a look at these today. > src/bhyve/bhyve_domain.c | 13 ++- > src/conf/domain_addr.c | 6 +- > src/conf/domain_conf.c | 174 ++++++++++----------------------- > src/conf/domain_conf.h | 16 +-- > src/hyperv/hyperv_driver.c | 28 ++---- > src/libxl/libxl_conf.c | 4 +- > src/libxl/libxl_domain.c | 11 +-- > src/libxl/libxl_driver.c | 11 +-- > src/libxl/xen_common.c | 15 +-- > src/libxl/xen_common.h | 2 +- > src/libxl/xen_xl.c | 4 +- > src/lxc/lxc_driver.c | 6 +- > src/qemu/qemu_domain_address.c | 8 +- > src/qemu/qemu_driver.c | 14 +-- > src/qemu/qemu_postparse.c | 58 ++++------- > src/qemu/qemu_process.c | 3 +- > src/vbox/vbox_common.c | 13 +-- > src/vmx/vmx.c | 12 +-- > src/vz/vz_driver.c | 11 +-- > src/vz/vz_sdk.c | 14 +-- > 20 files changed, 126 insertions(+), 297 deletions(-) > >-- >2.47.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 :|
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 :|
>
On Wed, Feb 26, 2025 at 04:46:34PM +0100, Martin Kletzander wrote:
> 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...
An alternative option is to port everything that uses virObject over
to use GObject, since virObject is just a thin back-compat shim.
With GObject there is no need to manually call functions to create
classes. There's just a static struct declared which triggers
creation of classes on first use. virIdentity is an example of an
object doing that.
Many are easy to change, but some of the larger objects will have
trouble as GObject artificially limits (public) struct size to 64k
IIRC, expecting you to have the split public/private struct design
where the private struct can be arbitrarily huge
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 :|
>>> On Wed, Feb 26, 2025 at 11:04:23AM +0100, Martin Kletzander wrote: >>>> The classic Hal fixing a lightbulb moment ;) At least more productive than spending an hour performing the "Hal looking for his glasses" routine (my variation usually concerns my phone, which is invariably in one of my pockets the entire time). >[...] On 2/26/25 10:53 AM, Daniel P. Berrangé wrote: > > An alternative option is to port everything that uses virObject over > to use GObject, since virObject is just a thin back-compat shim. I didn't pay attention at the time (or since) - is there 0 extra functionality in virObject that isn't in GObject (aside from the object size limitation you mention below)? > With GObject there is no need to manually call functions to create > classes. There's just a static struct declared which triggers > creation of classes on first use. virIdentity is an example of an > object doing that. > Many are easy to change, but some of the larger objects will have > trouble as GObject artificially limits (public) struct size to 64k > IIRC, expecting you to have the split public/private struct design > where the private struct can be arbitrarily huge Why didn't they just make the maximum struct size 640k? Bill's Law says that should be big enough for possible object.
© 2016 - 2025 Red Hat, Inc.