[PATCH 00/17] conf/qemu/etc: change several functions to return void instead of 0/-1
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
We haven't checked for memalloc failure in many years, and that was the only reason this function would have ever failed. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 15 +++++---------- src/conf/domain_conf.h | 2 +- src/libxl/libxl_domain.c | 5 +---- src/libxl/libxl_driver.c | 3 +-- src/lxc/lxc_driver.c | 3 +-- src/qemu/qemu_driver.c | 3 +-- src/qemu/qemu_process.c | 3 +-- 7 files changed, 11 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 45c2cd09f1..8751e1ef06 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14465,12 +14465,10 @@ virDomainChrTargetTypeToString(int deviceType, return type; } -int +void virDomainHostdevInsert(virDomainDef *def, virDomainHostdevDef *hostdev) { VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev); - - return 0; } virDomainHostdevDef * @@ -14886,9 +14884,8 @@ virDomainDiskRemoveByName(virDomainDef *def, const char *name) int virDomainNetInsert(virDomainDef *def, virDomainNetDef *net) { /* hostdev net devices must also exist in the hostdevs array */ - if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV && - virDomainHostdevInsert(def, &net->data.hostdev.def) < 0) - return -1; + if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) + virDomainHostdevInsert(def, &net->data.hostdev.def); VIR_APPEND_ELEMENT(def->nets, def->nnets, net); return 0; @@ -19281,10 +19278,8 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt, * where the actual network type is already known to be * hostdev) must also be in the hostdevs array. */ - if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV && - virDomainHostdevInsert(def, virDomainNetGetActualHostdev(net)) < 0) { - return NULL; - } + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) + virDomainHostdevInsert(def, virDomainNetGetActualHostdev(net)); } VIR_FREE(nodes); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ba1a495764..db49567313 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3994,7 +3994,7 @@ virDomainNetDef *virDomainNetRemove(virDomainDef *def, size_t i); virDomainNetDef *virDomainNetRemoveByObj(virDomainDef *def, virDomainNetDef *net); void virDomainNetRemoveHostdev(virDomainDef *def, virDomainNetDef *net); -int virDomainHostdevInsert(virDomainDef *def, virDomainHostdevDef *hostdev); +void virDomainHostdevInsert(virDomainDef *def, virDomainHostdevDef *hostdev); virDomainHostdevDef * virDomainHostdevRemove(virDomainDef *def, size_t i); int virDomainHostdevFind(virDomainDef *def, virDomainHostdevDef *match, diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index a049cdb30f..6805160923 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1014,10 +1014,7 @@ libxlNetworkPrepareDevices(virDomainDef *def) /* Each type='hostdev' network device must also have a * corresponding entry in the hostdevs array. */ - virDomainHostdevDef *hostdev = virDomainNetGetActualHostdev(net); - - if (virDomainHostdevInsert(def, hostdev) < 0) - return -1; + virDomainHostdevInsert(def, virDomainNetGetActualHostdev(net)); } } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 494b1ad9bc..f2bb6893c4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3590,8 +3590,7 @@ libxlDomainAttachDeviceConfig(virDomainDef *vmdef, virDomainDeviceDef *dev) return -1; } - if (virDomainHostdevInsert(vmdef, hostdev) < 0) - return -1; + virDomainHostdevInsert(vmdef, hostdev); dev->data.hostdev = NULL; break; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 4740aeed52..9301aedf58 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2992,8 +2992,7 @@ lxcDomainAttachDeviceConfig(virDomainDef *vmdef, _("device is already in the domain configuration")); return -1; } - if (virDomainHostdevInsert(vmdef, hostdev) < 0) - return -1; + virDomainHostdevInsert(vmdef, hostdev); dev->data.hostdev = NULL; ret = 0; break; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d2eddbd9ae..0a7c3588e2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6745,8 +6745,7 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef, _("device is already in the domain configuration")); return -1; } - if (virDomainHostdevInsert(vmdef, hostdev)) - return -1; + virDomainHostdevInsert(vmdef, hostdev); dev->data.hostdev = NULL; break; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6db48b0e7b..9cfd40993a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5909,8 +5909,7 @@ qemuProcessPrepareDomainNetwork(virDomainObj *vm) if (qemuDomainPrepareHostdev(hostdev, priv) < 0) return -1; - if (virDomainHostdevInsert(def, hostdev) < 0) - return -1; + virDomainHostdevInsert(def, hostdev); } } return 0; -- 2.47.1
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- src/libxl/libxl_driver.c | 3 +-- src/lxc/lxc_driver.c | 3 +-- src/qemu/qemu_driver.c | 5 +---- 5 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8751e1ef06..1882054c7b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14881,14 +14881,14 @@ virDomainDiskRemoveByName(virDomainDef *def, const char *name) return virDomainDiskRemove(def, idx); } -int virDomainNetInsert(virDomainDef *def, virDomainNetDef *net) +void +virDomainNetInsert(virDomainDef *def, virDomainNetDef *net) { /* hostdev net devices must also exist in the hostdevs array */ if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) virDomainHostdevInsert(def, &net->data.hostdev.def); VIR_APPEND_ELEMENT(def->nets, def->nnets, net); - return 0; } /** diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index db49567313..37298ca937 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3984,7 +3984,7 @@ int virDomainNetFindIdx(virDomainDef *def, virDomainNetDef *net); virDomainNetDef *virDomainNetFind(virDomainDef *def, const char *device); virDomainNetDef *virDomainNetFindByName(virDomainDef *def, const char *ifname); bool virDomainHasNet(virDomainDef *def, virDomainNetDef *net); -int virDomainNetInsert(virDomainDef *def, virDomainNetDef *net); +void virDomainNetInsert(virDomainDef *def, virDomainNetDef *net); void virDomainNetUpdate(virDomainDef *def, size_t netidx, virDomainNetDef *newnet); bool virDomainNetBackendIsEqual(virDomainNetBackend *src, virDomainNetBackend *dst); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index f2bb6893c4..45d15df8b8 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3565,8 +3565,7 @@ libxlDomainAttachDeviceConfig(virDomainDef *vmdef, virDomainDeviceDef *dev) virMacAddrFormat(&net->mac, mac)); return -1; } - if (virDomainNetInsert(vmdef, net)) - return -1; + virDomainNetInsert(vmdef, net); dev->data.net = NULL; break; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 9301aedf58..5795293079 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2979,8 +2979,7 @@ lxcDomainAttachDeviceConfig(virDomainDef *vmdef, case VIR_DOMAIN_DEVICE_NET: net = dev->data.net; - if (virDomainNetInsert(vmdef, net) < 0) - return -1; + virDomainNetInsert(vmdef, net); dev->data.net = NULL; ret = 0; break; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a7c3588e2..4a248cbafe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6699,7 +6699,6 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef, virDomainXMLOption *xmlopt) { virDomainDiskDef *disk; - virDomainNetDef *net; virDomainSoundDef *sound; virDomainHostdevDef *hostdev; virDomainLeaseDef *lease; @@ -6726,9 +6725,7 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef, break; case VIR_DOMAIN_DEVICE_NET: - net = dev->data.net; - if (virDomainNetInsert(vmdef, net)) - return -1; + virDomainNetInsert(vmdef, dev->data.net); dev->data.net = NULL; break; -- 2.47.1
On Tue, Feb 11, 2025 at 11:28:55PM -0500, Laine Stump wrote:
It can't fail.
I'm looking forward to this being the message in all the other commits =D
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- src/libxl/libxl_driver.c | 3 +-- src/lxc/lxc_driver.c | 3 +-- src/qemu/qemu_driver.c | 5 +---- 5 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a7c3588e2..4a248cbafe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6699,7 +6699,6 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef, virDomainXMLOption *xmlopt) { virDomainDiskDef *disk; - virDomainNetDef *net; virDomainSoundDef *sound; virDomainHostdevDef *hostdev; virDomainLeaseDef *lease; @@ -6726,9 +6725,7 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef, break;
case VIR_DOMAIN_DEVICE_NET: - net = dev->data.net; - if (virDomainNetInsert(vmdef, net)) - return -1; + virDomainNetInsert(vmdef, dev->data.net); dev->data.net = NULL;
or `virDomainNetInsert(vmdef, g_steal_pointer(dev->data.net))` either way Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
On 2/26/25 7:22 AM, Martin Kletzander wrote:
On Tue, Feb 11, 2025 at 11:28:55PM -0500, Laine Stump wrote:
It can't fail.
I'm looking forward to this being the message in all the other commits =D
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- src/libxl/libxl_driver.c | 3 +-- src/lxc/lxc_driver.c | 3 +-- src/qemu/qemu_driver.c | 5 +---- 5 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a7c3588e2..4a248cbafe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6699,7 +6699,6 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef, virDomainXMLOption *xmlopt) { virDomainDiskDef *disk; - virDomainNetDef *net; virDomainSoundDef *sound; virDomainHostdevDef *hostdev; virDomainLeaseDef *lease; @@ -6726,9 +6725,7 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef, break;
case VIR_DOMAIN_DEVICE_NET: - net = dev->data.net; - if (virDomainNetInsert(vmdef, net)) - return -1; + virDomainNetInsert(vmdef, dev->data.net); dev->data.net = NULL;
or `virDomainNetInsert(vmdef, g_steal_pointer(dev->data.net))`
Ah yes. I'd been too narrowed in to think about that. I like that much better (there were a few of those cases, weren't there? It's been a couple weeks since I did all these, so my memory is hazy).
either way
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Thanks!
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 4 +--- src/conf/domain_conf.h | 2 +- src/qemu/qemu_driver.c | 3 +-- src/vz/vz_sdk.c | 3 +-- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1882054c7b..b5b15b7768 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29205,12 +29205,10 @@ virDiskNameToBusDeviceIndex(virDomainDiskDef *disk, return 0; } -int +void virDomainFSInsert(virDomainDef *def, virDomainFSDef *fs) { VIR_APPEND_ELEMENT(def->fss, def->nfss, fs); - - return 0; } virDomainFSDef * diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 37298ca937..cb19f0fb29 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -4123,7 +4123,7 @@ int virDiskNameToBusDeviceIndex(virDomainDiskDef *disk, virDomainFSDef *virDomainGetFilesystemForTarget(virDomainDef *def, const char *target); -int virDomainFSInsert(virDomainDef *def, virDomainFSDef *fs); +void virDomainFSInsert(virDomainDef *def, virDomainFSDef *fs); int virDomainFSIndexByName(virDomainDef *def, const char *name); virDomainFSDef *virDomainFSRemove(virDomainDef *def, size_t i); ssize_t virDomainFSDefFind(virDomainDef *def, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4a248cbafe..d295f24cb3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6790,8 +6790,7 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef, return -1; } - if (virDomainFSInsert(vmdef, fs) < 0) - return -1; + virDomainFSInsert(vmdef, fs); dev->data.fs = NULL; break; diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index b20d454fb8..688c9d1ebc 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -801,8 +801,7 @@ prlsdkAddDomainHardDisksInfo(struct _vzDriver *driver, PRL_HANDLE sdkdom, virDom if (prlsdkGetFSInfo(hdd, fs) < 0) goto error; - if (virDomainFSInsert(def, fs) < 0) - goto error; + virDomainFSInsert(def, fs); fs = NULL; PrlHandle_Free(hdd); -- 2.47.1
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 4 +--- src/conf/domain_conf.h | 4 ++-- src/qemu/qemu_driver.c | 3 +-- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b5b15b7768..2e74f75a13 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15968,13 +15968,11 @@ virDomainRedirdevDefRemove(virDomainDef *def, size_t idx) } -int +void virDomainShmemDefInsert(virDomainDef *def, virDomainShmemDef *shmem) { VIR_APPEND_ELEMENT(def->shmems, def->nshmems, shmem); - - return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cb19f0fb29..0dd2d6f1d2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -4203,8 +4203,8 @@ virDomainMemoryFindByDeviceAlias(virDomainDef *def, const char *alias) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; -int virDomainShmemDefInsert(virDomainDef *def, virDomainShmemDef *shmem) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; +void virDomainShmemDefInsert(virDomainDef *def, virDomainShmemDef *shmem) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); bool virDomainShmemDefEquals(virDomainShmemDef *src, virDomainShmemDef *dst) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; ssize_t virDomainShmemDefFind(virDomainDef *def, virDomainShmemDef *shmem) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d295f24cb3..f1be2ca2d8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6824,8 +6824,7 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef, _("device is already in the domain configuration")); return -1; } - if (virDomainShmemDefInsert(vmdef, shmem) < 0) - return -1; + virDomainShmemDefInsert(vmdef, shmem); dev->data.shmem = NULL; break; -- 2.47.1
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 6 ++---- src/conf/domain_conf.h | 2 +- src/libxl/xen_common.c | 11 ++--------- src/qemu/qemu_postparse.c | 27 ++++++--------------------- src/vz/vz_driver.c | 11 ++--------- src/vz/vz_sdk.c | 11 ++--------- 6 files changed, 15 insertions(+), 53 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2e74f75a13..b14712997b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16548,7 +16548,7 @@ virDomainDefMaybeAddController(virDomainDef *def, } -int +void virDomainDefMaybeAddInput(virDomainDef *def, int type, int bus) @@ -16559,7 +16559,7 @@ virDomainDefMaybeAddInput(virDomainDef *def, for (i = 0; i < def->ninputs; i++) { if (def->inputs[i]->type == type && def->inputs[i]->bus == bus) - return 0; + return; } input = g_new0(virDomainInputDef, 1); @@ -16568,8 +16568,6 @@ virDomainDefMaybeAddInput(virDomainDef *def, input->bus = bus; VIR_APPEND_ELEMENT(def->inputs, def->ninputs, input); - - return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0dd2d6f1d2..1edc3679cd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -4370,7 +4370,7 @@ virDomainDefMaybeAddController(virDomainDef *def, virDomainControllerType type, int idx, int model); -int +void virDomainDefMaybeAddInput(virDomainDef *def, int type, int bus); diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 3a64f565f7..c9c8ed2fde 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -2400,15 +2400,8 @@ xenDomainDefAddImplicitInputDevice(virDomainDef *def) if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) implicitInputBus = VIR_DOMAIN_INPUT_BUS_PS2; - if (virDomainDefMaybeAddInput(def, - VIR_DOMAIN_INPUT_TYPE_MOUSE, - implicitInputBus) < 0) - return -1; - - if (virDomainDefMaybeAddInput(def, - VIR_DOMAIN_INPUT_TYPE_KBD, - implicitInputBus) < 0) - return -1; + virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_MOUSE, implicitInputBus); + virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_KBD, implicitInputBus); return 0; } diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c index 1f9077982a..53151eef75 100644 --- a/src/qemu/qemu_postparse.c +++ b/src/qemu/qemu_postparse.c @@ -1095,15 +1095,8 @@ qemuDomainDefAddImplicitInputDevice(virDomainDef *def, { if (virQEMUCapsSupportsI8042(qemuCaps, def) && def->features[VIR_DOMAIN_FEATURE_PS2] != VIR_TRISTATE_SWITCH_OFF) { - if (virDomainDefMaybeAddInput(def, - VIR_DOMAIN_INPUT_TYPE_MOUSE, - VIR_DOMAIN_INPUT_BUS_PS2) < 0) - return -1; - - if (virDomainDefMaybeAddInput(def, - VIR_DOMAIN_INPUT_TYPE_KBD, - VIR_DOMAIN_INPUT_BUS_PS2) < 0) - return -1; + virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_MOUSE, VIR_DOMAIN_INPUT_BUS_PS2); + virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_KBD, VIR_DOMAIN_INPUT_BUS_PS2); } return 0; @@ -1419,19 +1412,11 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, addDefaultUSBMouse = false; } - if (addDefaultUSBKBD && - def->ngraphics > 0 && - virDomainDefMaybeAddInput(def, - VIR_DOMAIN_INPUT_TYPE_KBD, - VIR_DOMAIN_INPUT_BUS_USB) < 0) - return -1; + if (addDefaultUSBKBD && def->ngraphics > 0) + virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_KBD, VIR_DOMAIN_INPUT_BUS_USB); - if (addDefaultUSBMouse && - def->ngraphics > 0 && - virDomainDefMaybeAddInput(def, - VIR_DOMAIN_INPUT_TYPE_MOUSE, - VIR_DOMAIN_INPUT_BUS_USB) < 0) - return -1; + if (addDefaultUSBMouse && def->ngraphics > 0) + virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_MOUSE, VIR_DOMAIN_INPUT_BUS_USB); if (addPanicDevice) { virDomainPanicModel defaultModel = qemuDomainDefaultPanicModel(def); diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 4edea4bf18..571735f940 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -219,15 +219,8 @@ vzDomainDefAddDefaultInputDevices(virDomainDef *def) if (def->ngraphics == 0) return 0; - if (virDomainDefMaybeAddInput(def, - VIR_DOMAIN_INPUT_TYPE_MOUSE, - bus) < 0) - return -1; - - if (virDomainDefMaybeAddInput(def, - VIR_DOMAIN_INPUT_TYPE_KBD, - bus) < 0) - return -1; + virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_MOUSE, bus); + virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_KBD, bus); return 0; } diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 688c9d1ebc..c64d0b73e5 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1848,15 +1848,8 @@ prlsdkLoadDomain(struct _vzDriver *driver, int bus = IS_CT(def) ? VIR_DOMAIN_INPUT_BUS_PARALLELS : VIR_DOMAIN_INPUT_BUS_PS2; - if (virDomainDefMaybeAddInput(def, - VIR_DOMAIN_INPUT_TYPE_MOUSE, - bus) < 0) - return NULL; - - if (virDomainDefMaybeAddInput(def, - VIR_DOMAIN_INPUT_TYPE_KBD, - bus) < 0) - return NULL; + virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_MOUSE, bus); + virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_KBD, bus); } if (!dom) { -- 2.47.1
On Tue, Feb 11, 2025 at 11:28:58PM -0500, Laine Stump wrote:
It can't fail.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
--- src/conf/domain_conf.c | 6 ++---- src/conf/domain_conf.h | 2 +- src/libxl/xen_common.c | 11 ++--------- src/qemu/qemu_postparse.c | 27 ++++++--------------------- src/vz/vz_driver.c | 11 ++--------- src/vz/vz_sdk.c | 11 ++--------- 6 files changed, 15 insertions(+), 53 deletions(-)
15+, 53-, nice ;)
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/libxl/libxl_domain.c | 3 +-- src/libxl/xen_common.c | 4 +--- src/libxl/xen_common.h | 2 +- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 6805160923..c7ba5636c4 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -266,8 +266,7 @@ libxlDomainDefPostParse(virDomainDef *def, } /* add implicit input devices */ - if (xenDomainDefAddImplicitInputDevice(def) < 0) - return -1; + xenDomainDefAddImplicitInputDevice(def); /* For x86_64 HVM */ if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index c9c8ed2fde..d82e33600f 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -2392,7 +2392,7 @@ xenFormatConfigCommon(virConf *conf, } -int +void xenDomainDefAddImplicitInputDevice(virDomainDef *def) { virDomainInputBus implicitInputBus = VIR_DOMAIN_INPUT_BUS_XEN; @@ -2402,6 +2402,4 @@ xenDomainDefAddImplicitInputDevice(virDomainDef *def) virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_MOUSE, implicitInputBus); virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_KBD, implicitInputBus); - - return 0; } diff --git a/src/libxl/xen_common.h b/src/libxl/xen_common.h index 95408fa896..c52a4f1232 100644 --- a/src/libxl/xen_common.h +++ b/src/libxl/xen_common.h @@ -65,4 +65,4 @@ int xenFormatConfigCommon(virConf *conf, char *xenMakeIPList(virNetDevIPInfo *guestIP); -int xenDomainDefAddImplicitInputDevice(virDomainDef *def); +void xenDomainDefAddImplicitInputDevice(virDomainDef *def); -- 2.47.1
On Tue, Feb 11, 2025 at 11:28:59PM -0500, Laine Stump wrote:
It can't fail.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_postparse.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c index 53151eef75..0de48916f4 100644 --- a/src/qemu/qemu_postparse.c +++ b/src/qemu/qemu_postparse.c @@ -1089,7 +1089,7 @@ qemuDomainDefBootPostParse(virDomainDef *def, } -static int +static void qemuDomainDefAddImplicitInputDevice(virDomainDef *def, virQEMUCaps *qemuCaps) { @@ -1098,8 +1098,6 @@ qemuDomainDefAddImplicitInputDevice(virDomainDef *def, virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_MOUSE, VIR_DOMAIN_INPUT_BUS_PS2); virDomainDefMaybeAddInput(def, VIR_DOMAIN_INPUT_TYPE_KBD, VIR_DOMAIN_INPUT_BUS_PS2); } - - return 0; } @@ -1194,8 +1192,7 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, bool addIOMMU = false; /* add implicit input devices */ - if (qemuDomainDefAddImplicitInputDevice(def, qemuCaps) < 0) - return -1; + qemuDomainDefAddImplicitInputDevice(def, qemuCaps); /* Add implicit PCI root controller if the machine has one */ switch (def->os.arch) { -- 2.47.1
On Tue, Feb 11, 2025 at 11:29:00PM -0500, Laine Stump wrote:
It can't fail.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
It can't fail, so the caller doesn't need to check the return. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 8 ++------ src/hyperv/hyperv_driver.c | 6 +----- src/libxl/libxl_conf.c | 4 +--- src/libxl/libxl_driver.c | 5 +---- src/libxl/xen_xl.c | 4 +--- 5 files changed, 6 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b14712997b..ce7501f2b3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8571,8 +8571,7 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt, VIR_XML_PROP_NONE, &type) < 0) return NULL; - if (!(def = virDomainControllerDefNew(type))) - return NULL; + def = virDomainControllerDefNew(type); if ((model = virXMLPropString(node, "model"))) { if ((def->model = virDomainControllerModelTypeFromString(def, model)) < 0) { @@ -16459,10 +16458,7 @@ virDomainDefAddController(virDomainDef *def, int idx, int model) { - virDomainControllerDef *cont; - - if (!(cont = virDomainControllerDefNew(type))) - return NULL; + virDomainControllerDef *cont = virDomainControllerDefNew(type); if (idx < 0) idx = virDomainControllerFindUnusedIndex(def, type); diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 43ccb9cbd7..66286cc756 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1174,13 +1174,9 @@ hypervDomainDefAppendController(virDomainDef *def, int idx, virDomainControllerType controllerType) { - virDomainControllerDef *controller = NULL; - - if (!(controller = virDomainControllerDefNew(controllerType))) - return -1; + virDomainControllerDef *controller = virDomainControllerDefNew(controllerType); controller->idx = idx; - VIR_APPEND_ELEMENT(def->controllers, def->ncontrollers, controller); return 0; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index c404226e43..e06655605b 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -2102,9 +2102,7 @@ libxlMakeDefaultUSBControllers(virDomainDef *def, x_controllers = g_new0(libxl_device_usbctrl, ncontrollers); for (i = 0; i < ncontrollers; i++) { - if (!(l_controller = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB))) - goto error; - + l_controller = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB); l_controller->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2; l_controller->idx = i; l_controller->opts.usbopts.ports = 8; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 45d15df8b8..7d214a07d3 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3221,10 +3221,7 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivate *driver, if (ports <= usbdevs) { /* no free ports, we will create a new usb controller */ - virDomainControllerDef *controller; - - if (!(controller = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB))) - goto cleanup; + virDomainControllerDef *controller = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB); controller->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2; controller->idx = -1; diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 53f6871efc..30dffcac20 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -861,9 +861,7 @@ xenParseXLUSBController(virConf *conf, virDomainDef *def) else usbctrl_type = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2; - if (!(controller = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB))) - return -1; - + controller = virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB); controller->type = VIR_DOMAIN_CONTROLLER_TYPE_USB; controller->model = usbctrl_type; controller->opts.usbopts.ports = usbctrl_ports; -- 2.47.1
On Tue, Feb 11, 2025 at 11:29:01PM -0500, Laine Stump wrote:
It can't fail, so the caller doesn't need to check the return.
My dream broke, the perfect message is no longer, disappointed ;) On the other hand this commit by itself eliminates so many parentheses that it looks like we ditched lisp from the codebase =D
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
It can't fail, so the caller doesn't need to check the return. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 25 +++++++++---------------- src/libxl/libxl_domain.c | 3 +-- src/qemu/qemu_postparse.c | 12 ++++++------ src/vbox/vbox_common.c | 13 ++----------- src/vmx/vmx.c | 12 ++++-------- 5 files changed, 22 insertions(+), 43 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ce7501f2b3..837b306919 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16490,10 +16490,7 @@ virDomainDefAddUSBController(virDomainDef *def, int idx, int model) { virDomainControllerDef *cont; /* this is a *copy* of the virDomainControllerDef */ - cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, - idx, model); - if (!cont) - return -1; + cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, idx, model); if (model != VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1) return 0; @@ -16504,21 +16501,18 @@ virDomainDefAddUSBController(virDomainDef *def, int idx, int model) idx = cont->idx; /* in case original request was "-1" */ - if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, - idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1))) - return -1; + cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, + idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1); cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB; cont->info.master.usb.startport = 0; - if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, - idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2))) - return -1; + cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, + idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2); cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB; cont->info.master.usb.startport = 2; - if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, - idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3))) - return -1; + cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, + idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3); cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB; cont->info.master.usb.startport = 4; @@ -16538,9 +16532,8 @@ virDomainDefMaybeAddController(virDomainDef *def, if (idx >= 0 && virDomainControllerFind(def, type, idx) >= 0) return 0; - if (virDomainDefAddController(def, type, idx, model)) - return 1; - return -1; + virDomainDefAddController(def, type, idx, model); + return 1; } diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index c7ba5636c4..596503c09f 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -291,8 +291,7 @@ libxlDomainDefPostParse(virDomainDef *def, /* add implicit xenbus device */ if (virDomainControllerFindByType(def, VIR_DOMAIN_CONTROLLER_TYPE_XENBUS) == -1) - if (virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_XENBUS, -1, -1) == NULL) - return -1; + virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_XENBUS, -1, -1); return 0; } diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c index 0de48916f4..71d772bfa0 100644 --- a/src/qemu/qemu_postparse.c +++ b/src/qemu/qemu_postparse.c @@ -1354,9 +1354,9 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainControllerModelPCITypeToString(def->controllers[pciRoot]->model)); return -1; } - } else if (!virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, - VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)) { - return -1; + } else { + virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT); } } @@ -1375,9 +1375,9 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, virDomainControllerModelPCITypeToString(def->controllers[pciRoot]->model)); return -1; } - } else if (!virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, - VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)) { - return -1; + } else { + virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT); } } diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index de3c9989a5..4387d706d6 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3230,17 +3230,8 @@ vboxDumpStorageControllers(virDomainDef *def, IMachine *machine) goto cleanup; } - if (type != VIR_DOMAIN_CONTROLLER_TYPE_LAST) { - virDomainControllerDef *cont; - - cont = virDomainDefAddController(def, type, -1, model); - if (!cont) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to add %1$s controller type definition"), - virDomainControllerTypeToString(type)); - goto cleanup; - } - } + if (type != VIR_DOMAIN_CONTROLLER_TYPE_LAST) + virDomainDefAddController(def, type, -1, model); } ret = 0; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 23a8a35360..8d10b6e9e8 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1756,9 +1756,8 @@ virVMXParseConfig(virVMXContext *ctx, if (def->ndisks != 0) { virDomainDeviceInfo *info = &def->disks[def->ndisks - 1]->info; for (controller = 0; controller <= info->addr.drive.controller; controller++) { - if (!virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, - controller, scsi_virtualDev[controller])) - goto cleanup; + virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, + controller, scsi_virtualDev[controller]); } saved_ndisks = def->ndisks; } @@ -1799,11 +1798,8 @@ virVMXParseConfig(virVMXContext *ctx, * currently used by a disk */ if (def->ndisks - saved_ndisks != 0) { virDomainDeviceInfo *info = &def->disks[def->ndisks - 1]->info; - for (controller = 0; controller <= info->addr.drive.controller; controller++) { - if (!virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, - controller, -1)) - goto cleanup; - } + for (controller = 0; controller <= info->addr.drive.controller; controller++) + virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, controller, -1); } /* def:disks (ide) */ -- 2.47.1
On Tue, Feb 11, 2025 at 11:29:02PM -0500, Laine Stump wrote:
It can't fail, so the caller doesn't need to check the return.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 8 +++----- src/conf/domain_conf.h | 2 +- src/qemu/qemu_postparse.c | 6 ++---- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 837b306919..5d887054af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16483,9 +16483,9 @@ virDomainDefAddController(virDomainDef *def, * also add companion uhci1, uhci2, and uhci3 controllers at the same * index. * - * Returns 0 on success, -1 on failure. + * Always succeeds. */ -int +void virDomainDefAddUSBController(virDomainDef *def, int idx, int model) { virDomainControllerDef *cont; /* this is a *copy* of the virDomainControllerDef */ @@ -16493,7 +16493,7 @@ virDomainDefAddUSBController(virDomainDef *def, int idx, int model) cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, idx, model); if (model != VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1) - return 0; + return; /* When the initial controller is ich9-usb-ehci, also add the * companion controllers @@ -16515,8 +16515,6 @@ virDomainDefAddUSBController(virDomainDef *def, int idx, int model) idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3); cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB; cont->info.master.usb.startport = 4; - - return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1edc3679cd..87774b7dbc 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -4363,7 +4363,7 @@ virDomainDefAddController(virDomainDef *def, virDomainControllerType type, int idx, int model); -int +void virDomainDefAddUSBController(virDomainDef *def, int idx, int model); int virDomainDefMaybeAddController(virDomainDef *def, diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c index 71d772bfa0..0eb6a81f94 100644 --- a/src/qemu/qemu_postparse.c +++ b/src/qemu/qemu_postparse.c @@ -1331,10 +1331,8 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, break; } - if (addDefaultUSB && - virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0) < 0 && - virDomainDefAddUSBController(def, 0, usbModel) < 0) - return -1; + if (addDefaultUSB && virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0) < 0) + virDomainDefAddUSBController(def, 0, usbModel); if (addImplicitSATA && virDomainDefMaybeAddController( -- 2.47.1
On Tue, Feb 11, 2025 at 11:29:03PM -0500, Laine Stump wrote:
It can't fail.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 8 +++----- src/conf/domain_conf.h | 2 +- src/qemu/qemu_postparse.c | 6 ++---- 3 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 837b306919..5d887054af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16483,9 +16483,9 @@ virDomainDefAddController(virDomainDef *def, * also add companion uhci1, uhci2, and uhci3 controllers at the same * index. * - * Returns 0 on success, -1 on failure. + * Always succeeds.
Or just remove this line completely. Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
It can't fail. And as a result, hypervDomainDefAppendSCSIController() and hypervDomainDefAppendIDEController() can also be changed to return void. Signed-off-by: Laine Stump <laine@redhat.com> --- src/hyperv/hyperv_driver.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 66286cc756..0d1e388c08 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1169,7 +1169,7 @@ hypervDomainAttachSyntheticEthernetAdapter(virDomainPtr domain, /* * Functions for deserializing device entries */ -static int +static void hypervDomainDefAppendController(virDomainDef *def, int idx, virDomainControllerType controllerType) @@ -1178,22 +1178,20 @@ hypervDomainDefAppendController(virDomainDef *def, controller->idx = idx; VIR_APPEND_ELEMENT(def->controllers, def->ncontrollers, controller); - - return 0; } -static int +static void hypervDomainDefAppendIDEController(virDomainDef *def) { - return hypervDomainDefAppendController(def, 0, VIR_DOMAIN_CONTROLLER_TYPE_IDE); + hypervDomainDefAppendController(def, 0, VIR_DOMAIN_CONTROLLER_TYPE_IDE); } -static int +static void hypervDomainDefAppendSCSIController(virDomainDef *def, int idx) { - return hypervDomainDefAppendController(def, idx, VIR_DOMAIN_CONTROLLER_TYPE_SCSI); + hypervDomainDefAppendController(def, idx, VIR_DOMAIN_CONTROLLER_TYPE_SCSI); } @@ -1460,18 +1458,12 @@ hypervDomainDefParseStorage(hypervPrivate *priv, ideChannels[channel] = entry; if (!hasIdeController) { /* Hyper-V represents its PIIX4 controller's two channels as separate objects. */ - if (hypervDomainDefAppendIDEController(def) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add IDE controller")); - return -1; - } + hypervDomainDefAppendIDEController(def); hasIdeController = true; } } else if (entry->data->ResourceType == MSVM_RASD_RESOURCETYPE_PARALLEL_SCSI_HBA) { scsiControllers[scsi_idx++] = entry; - if (hypervDomainDefAppendSCSIController(def, scsi_idx - 1) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not parse SCSI controller")); - return -1; - } + hypervDomainDefAppendSCSIController(def, scsi_idx - 1); } entry = entry->next; -- 2.47.1
On Tue, Feb 11, 2025 at 11:29:04PM -0500, Laine Stump wrote:
It can't fail. And as a result, hypervDomainDefAppendSCSIController() and hypervDomainDefAppendIDEController() can also be changed to return void.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
This function can't fail, but it has always returned 1 if a controller is added and 0 if not, and there is one place that checks for a 1 return, so we remove the -1 return and change it to return true/false instead of 1/0. Signed-off-by: Laine Stump <laine@redhat.com> --- src/bhyve/bhyve_domain.c | 13 ++++++------- src/conf/domain_addr.c | 6 +----- src/conf/domain_conf.c | 34 ++++++++++------------------------ src/conf/domain_conf.h | 2 +- src/qemu/qemu_domain_address.c | 8 +------- src/qemu/qemu_postparse.c | 6 ++---- 6 files changed, 21 insertions(+), 48 deletions(-) diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index 684d870749..7d1ea7f1b1 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -94,14 +94,13 @@ bhyveDomainDefPostParse(virDomainDef *def, return -1; /* Add an implicit PCI root controller */ - if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, - VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) - return -1; + virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT); - if (bhyveDomainDefNeedsISAController(def)) - if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_ISA, 0, - VIR_DOMAIN_CONTROLLER_MODEL_ISA_DEFAULT) < 0) - return -1; + if (bhyveDomainDefNeedsISAController(def)) { + virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_ISA, 0, + VIR_DOMAIN_CONTROLLER_MODEL_ISA_DEFAULT); + } return 0; } diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index a53ff6df6c..8dfa8feca0 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1625,11 +1625,7 @@ virDomainVirtioSerialAddrSetAutoaddController(virDomainDef *def, { int contidx; - if (virDomainDefMaybeAddController(def, - VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, - idx, -1) < 0) - return -1; - + virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, idx, -1); contidx = virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, idx); if (virDomainVirtioSerialAddrSetAddController(addrs, def->controllers[contidx]) < 0) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5d887054af..611b6a44c8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16518,7 +16518,7 @@ virDomainDefAddUSBController(virDomainDef *def, int idx, int model) } -int +bool virDomainDefMaybeAddController(virDomainDef *def, virDomainControllerType type, int idx, @@ -16528,10 +16528,10 @@ virDomainDefMaybeAddController(virDomainDef *def, * in use for that type of controller */ if (idx >= 0 && virDomainControllerFind(def, type, idx) >= 0) - return 0; + return false; virDomainDefAddController(def, type, idx, model); - return 1; + return true; } @@ -17229,11 +17229,8 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDef *def) if (maxController == -1) return 0; - for (i = 0; i <= maxController; i++) { - if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, - i, newModel) < 0) - return -1; - } + for (i = 0; i <= maxController; i++) + virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, newModel); return 0; } @@ -22271,10 +22268,8 @@ virDomainDefAddDiskControllersForType(virDomainDef *def, if (maxController == -1) return 0; - for (i = 0; i <= maxController; i++) { - if (virDomainDefMaybeAddController(def, controllerType, i, -1) < 0) - return -1; - } + for (i = 0; i <= maxController; i++) + virDomainDefMaybeAddController(def, controllerType, i, -1); return 0; } @@ -22294,10 +22289,7 @@ virDomainDefMaybeAddVirtioSerialController(virDomainDef *def) if (channel->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL) idx = channel->info.addr.vioserial.controller; - if (virDomainDefMaybeAddController(def, - VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, - idx, -1) < 0) - return -1; + virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, idx, -1); } } @@ -22309,10 +22301,7 @@ virDomainDefMaybeAddVirtioSerialController(virDomainDef *def) if (console->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL) idx = console->info.addr.vioserial.controller; - if (virDomainDefMaybeAddController(def, - VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, - idx, -1) < 0) - return -1; + virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, idx, -1); } } @@ -22349,10 +22338,7 @@ virDomainDefMaybeAddSmartcardController(virDomainDef *def) smartcard->info.addr.ccid.slot = max + 1; } - if (virDomainDefMaybeAddController(def, - VIR_DOMAIN_CONTROLLER_TYPE_CCID, - idx, -1) < 0) - return -1; + virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_CCID, idx, -1); } return 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 87774b7dbc..78a9ae5749 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -4365,7 +4365,7 @@ virDomainDefAddController(virDomainDef *def, int model); void virDomainDefAddUSBController(virDomainDef *def, int idx, int model); -int +bool virDomainDefMaybeAddController(virDomainDef *def, virDomainControllerType type, int idx, diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 970ae3949d..d38983bf63 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -2620,7 +2620,6 @@ qemuDomainAssignPCIAddresses(virDomainDef *def, int max_idx = -1; int nbuses = 0; size_t i; - int rv; for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDef *cont = def->controllers[i]; @@ -2737,12 +2736,7 @@ qemuDomainAssignPCIAddresses(virDomainDef *def, int contIndex; virDomainPCIAddressBus *bus = &addrs->buses[i]; - if ((rv = virDomainDefMaybeAddController( - def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, - i, bus->model)) < 0) - goto cleanup; - - if (rv == 0) + if (!virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, i, bus->model)) continue; /* no new controller added */ /* We did add a new controller, so we will need one more diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c index 0eb6a81f94..fc7918d9c4 100644 --- a/src/qemu/qemu_postparse.c +++ b/src/qemu/qemu_postparse.c @@ -1334,10 +1334,8 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, if (addDefaultUSB && virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0) < 0) virDomainDefAddUSBController(def, 0, usbModel); - if (addImplicitSATA && - virDomainDefMaybeAddController( - def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, 0, -1) < 0) - return -1; + if (addImplicitSATA) + virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, 0, -1); pciRoot = virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0); -- 2.47.1
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 611b6a44c8..d3e95a2f92 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17200,7 +17200,7 @@ virDomainFeaturesDefParse(virDomainDef *def, } -static int +static void virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDef *def) { /* Look for any hostdev scsi dev */ @@ -17227,12 +17227,10 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDef *def) } if (maxController == -1) - return 0; + return; for (i = 0; i <= maxController; i++) virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, newModel); - - return 0; } @@ -19497,8 +19495,7 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt, * post processing) because that will result in the failure to * load the controller during hostdev hotplug. */ - if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) - return NULL; + virDomainDefMaybeAddHostdevSCSIcontroller(def); } VIR_FREE(nodes); @@ -22379,8 +22376,7 @@ virDomainDefAddImplicitControllers(virDomainDef *def) if (virDomainDefMaybeAddSmartcardController(def) < 0) return -1; - if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) - return -1; + virDomainDefMaybeAddHostdevSCSIcontroller(def); return 0; } -- 2.47.1
On Tue, Feb 11, 2025 at 11:29:06PM -0500, Laine Stump wrote:
It can't fail.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 611b6a44c8..d3e95a2f92 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17200,7 +17200,7 @@ virDomainFeaturesDefParse(virDomainDef *def, }
-static int +static void virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDef *def) { /* Look for any hostdev scsi dev */ @@ -17227,12 +17227,10 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDef *def) }
if (maxController == -1) - return 0; + return;
If `i` was ssize_t instead of size_t, you could've even removed the above condition, so maybe I'm glad that it's size_t because that for loop below would then gross me out =D Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d3e95a2f92..c056abb28f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22243,7 +22243,7 @@ virDomainDefCheckABIStability(virDomainDef *src, } -static int +static void virDomainDefAddDiskControllersForType(virDomainDef *def, virDomainControllerType controllerType, int diskBus) @@ -22263,12 +22263,10 @@ virDomainDefAddDiskControllersForType(virDomainDef *def, } if (maxController == -1) - return 0; + return; for (i = 0; i <= maxController; i++) virDomainDefMaybeAddController(def, controllerType, i, -1); - - return 0; } @@ -22350,25 +22348,14 @@ virDomainDefMaybeAddSmartcardController(virDomainDef *def) static int virDomainDefAddImplicitControllers(virDomainDef *def) { - if (virDomainDefAddDiskControllersForType(def, - VIR_DOMAIN_CONTROLLER_TYPE_SCSI, - VIR_DOMAIN_DISK_BUS_SCSI) < 0) - return -1; - - if (virDomainDefAddDiskControllersForType(def, - VIR_DOMAIN_CONTROLLER_TYPE_FDC, - VIR_DOMAIN_DISK_BUS_FDC) < 0) - return -1; - - if (virDomainDefAddDiskControllersForType(def, - VIR_DOMAIN_CONTROLLER_TYPE_IDE, - VIR_DOMAIN_DISK_BUS_IDE) < 0) - return -1; - - if (virDomainDefAddDiskControllersForType(def, - VIR_DOMAIN_CONTROLLER_TYPE_SATA, - VIR_DOMAIN_DISK_BUS_SATA) < 0) - return -1; + virDomainDefAddDiskControllersForType(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, + VIR_DOMAIN_DISK_BUS_SCSI); + virDomainDefAddDiskControllersForType(def, VIR_DOMAIN_CONTROLLER_TYPE_FDC, + VIR_DOMAIN_DISK_BUS_FDC); + virDomainDefAddDiskControllersForType(def, VIR_DOMAIN_CONTROLLER_TYPE_IDE, + VIR_DOMAIN_DISK_BUS_IDE); + virDomainDefAddDiskControllersForType(def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, + VIR_DOMAIN_DISK_BUS_SATA); if (virDomainDefMaybeAddVirtioSerialController(def) < 0) return -1; -- 2.47.1
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c056abb28f..fe816dfd6d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22270,7 +22270,7 @@ virDomainDefAddDiskControllersForType(virDomainDef *def, } -static int +static void virDomainDefMaybeAddVirtioSerialController(virDomainDef *def) { /* Look for any virtio serial or virtio console devs */ @@ -22299,8 +22299,6 @@ virDomainDefMaybeAddVirtioSerialController(virDomainDef *def) virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, idx, -1); } } - - return 0; } @@ -22357,8 +22355,7 @@ virDomainDefAddImplicitControllers(virDomainDef *def) virDomainDefAddDiskControllersForType(def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, VIR_DOMAIN_DISK_BUS_SATA); - if (virDomainDefMaybeAddVirtioSerialController(def) < 0) - return -1; + virDomainDefMaybeAddVirtioSerialController(def); if (virDomainDefMaybeAddSmartcardController(def) < 0) return -1; -- 2.47.1
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fe816dfd6d..c4bdf58788 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22302,7 +22302,7 @@ virDomainDefMaybeAddVirtioSerialController(virDomainDef *def) } -static int +static void virDomainDefMaybeAddSmartcardController(virDomainDef *def) { /* Look for any smartcard devs */ @@ -22333,8 +22333,6 @@ virDomainDefMaybeAddSmartcardController(virDomainDef *def) virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_CCID, idx, -1); } - - return 0; } /* @@ -22356,10 +22354,7 @@ virDomainDefAddImplicitControllers(virDomainDef *def) VIR_DOMAIN_DISK_BUS_SATA); virDomainDefMaybeAddVirtioSerialController(def); - - if (virDomainDefMaybeAddSmartcardController(def) < 0) - return -1; - + virDomainDefMaybeAddSmartcardController(def); virDomainDefMaybeAddHostdevSCSIcontroller(def); return 0; -- 2.47.1
It can't fail. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c4bdf58788..21fe1502db 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22341,7 +22341,7 @@ virDomainDefMaybeAddSmartcardController(virDomainDef *def) * in the XML. This is for compat with existing apps which will * not know/care about <controller> info in the XML */ -static int +static void virDomainDefAddImplicitControllers(virDomainDef *def) { virDomainDefAddDiskControllersForType(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, @@ -22356,8 +22356,6 @@ virDomainDefAddImplicitControllers(virDomainDef *def) virDomainDefMaybeAddVirtioSerialController(def); virDomainDefMaybeAddSmartcardController(def); virDomainDefMaybeAddHostdevSCSIcontroller(def); - - return 0; } static int @@ -22384,8 +22382,7 @@ virDomainDefAddImplicitDevices(virDomainDef *def, virDomainXMLOption *xmlopt) if (virDomainDefAddConsoleCompat(def) < 0) return -1; } - if (virDomainDefAddImplicitControllers(def) < 0) - return -1; + virDomainDefAddImplicitControllers(def); if (virDomainDefAddImplicitVideo(def, xmlopt) < 0) return -1; -- 2.47.1
On Tue, Feb 11, 2025 at 11:29:10PM -0500, Laine Stump wrote:
It can't fail.
Signed-off-by: Laine Stump <laine@redhat.com>
Patches 14-17 are also Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
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.
participants (3)
-
Daniel P. Berrangé -
Laine Stump -
Martin Kletzander