[PATCH] domain_conf: make virDomainDiskSetSource() void
The function only returns zero or aborts, so it might as well be void. This has the added benefit of simplifying the code that calls it. Signed-off-by: Matt Coleman <matt@datto.com> --- src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 3 +-- src/libxl/libxl_driver.c | 6 ++---- src/libxl/xen_xl.c | 3 +-- src/libxl/xen_xm.c | 13 ++++--------- src/lxc/lxc_controller.c | 13 ++----------- src/vbox/vbox_common.c | 6 +----- src/vmx/vmx.c | 36 ++++++++++++------------------------ src/vz/vz_sdk.c | 4 ++-- 9 files changed, 26 insertions(+), 61 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7696b12ef9..5c8ec19da8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2241,13 +2241,12 @@ virDomainDiskGetSource(virDomainDiskDef const *def) } -int +void virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src) { char *tmp = g_strdup(src); g_free(def->src->path); def->src->path = tmp; - return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8a487e9de3..c164b28ea1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3040,8 +3040,7 @@ void virDomainLeaseDefFree(virDomainLeaseDefPtr def); int virDomainDiskGetType(virDomainDiskDefPtr def); void virDomainDiskSetType(virDomainDiskDefPtr def, int type); const char *virDomainDiskGetSource(virDomainDiskDef const *def); -int virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src) - G_GNUC_WARN_UNUSED_RESULT; +void virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src); void virDomainDiskEmptySource(virDomainDiskDefPtr def); const char *virDomainDiskGetDriver(const virDomainDiskDef *def); int virDomainDiskSetDriver(virDomainDiskDefPtr def, const char *name) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 824ed60dfd..00a74dcb49 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3022,8 +3022,7 @@ libxlDomainChangeEjectableMedia(virDomainObjPtr vm, virDomainDiskDefPtr disk) goto cleanup; } - if (virDomainDiskSetSource(origdisk, virDomainDiskGetSource(disk)) < 0) - goto cleanup; + virDomainDiskSetSource(origdisk, virDomainDiskGetSource(disk)); virDomainDiskSetType(origdisk, virDomainDiskGetType(disk)); virDomainDiskDefFree(disk); @@ -4084,8 +4083,7 @@ libxlDomainUpdateDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) return -1; } - if (virDomainDiskSetSource(orig, virDomainDiskGetSource(disk)) < 0) - return -1; + virDomainDiskSetSource(orig, virDomainDiskGetSource(disk)); virDomainDiskSetType(orig, virDomainDiskGetType(disk)); virDomainDiskSetFormat(orig, virDomainDiskGetFormat(disk)); if (virDomainDiskSetDriver(orig, virDomainDiskGetDriver(disk)) < 0) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 74f48e4117..d195f866c5 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -641,8 +641,7 @@ xenParseXLDiskSrc(virDomainDiskDefPtr disk, char *srcstr) disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_RBD; ret = virStorageSourceParseRBDColonString(tmpstr, disk->src); } else { - if (virDomainDiskSetSource(disk, srcstr) < 0) - goto cleanup; + virDomainDiskSetSource(disk, srcstr); ret = 0; } diff --git a/src/libxl/xen_xm.c b/src/libxl/xen_xm.c index 9063a43135..283ed1ee8b 100644 --- a/src/libxl/xen_xm.c +++ b/src/libxl/xen_xm.c @@ -129,14 +129,11 @@ xenParseXMDisk(char *entry, int hvm) if (offset == head) { /* No source file given, eg CDROM with no media */ - ignore_value(virDomainDiskSetSource(disk, NULL)); + virDomainDiskSetSource(disk, NULL); } else { tmp = g_strndup(head, offset - head); - if (virDomainDiskSetSource(disk, tmp) < 0) { - VIR_FREE(tmp); - goto error; - } + virDomainDiskSetSource(disk, tmp); VIR_FREE(tmp); } @@ -175,8 +172,7 @@ xenParseXMDisk(char *entry, int hvm) VIR_FREE(tmp); /* Strip the prefix we found off the source file name */ - if (virDomainDiskSetSource(disk, src + len + 1) < 0) - goto error; + virDomainDiskSetSource(disk, src + len + 1); src = virDomainDiskGetSource(disk); } @@ -206,8 +202,7 @@ xenParseXMDisk(char *entry, int hvm) } /* Strip the prefix we found off the source file name */ - if (virDomainDiskSetSource(disk, src + len + 1) < 0) - goto error; + virDomainDiskSetSource(disk, src + len + 1); src = virDomainDiskGetSource(disk); } } diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 4f77a6ace8..97de0408b6 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -479,7 +479,6 @@ static int virLXCControllerSetupLoopDeviceDisk(virDomainDiskDefPtr disk) int lofd; g_autofree char *loname = NULL; const char *src = virDomainDiskGetSource(disk); - int ret = -1; if ((lofd = virFileLoopDeviceAssociate(src, &loname)) < 0) return -1; @@ -492,14 +491,7 @@ static int virLXCControllerSetupLoopDeviceDisk(virDomainDiskDefPtr disk) * the rest of container setup 'just works' */ virDomainDiskSetType(disk, VIR_STORAGE_TYPE_BLOCK); - if (virDomainDiskSetSource(disk, loname) < 0) - goto cleanup; - - ret = 0; - - cleanup: - if (ret < 0) - VIR_FORCE_CLOSE(lofd); + virDomainDiskSetSource(disk, loname); return lofd; @@ -561,8 +553,7 @@ static int virLXCControllerSetupNBDDeviceDisk(virDomainDiskDefPtr disk) * the rest of container setup 'just works' */ virDomainDiskSetType(disk, VIR_STORAGE_TYPE_BLOCK); - if (virDomainDiskSetSource(disk, dev) < 0) - return -1; + virDomainDiskSetSource(disk, dev); return 0; } diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index d072ca8015..26168c3c5b 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3309,11 +3309,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); - if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Could not set disk source")); - goto cleanup; - } + virDomainDiskSetSource(disk, mediumLocUtf8); rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); if (NS_FAILED(rc)) { diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 5fac290a5e..4001174380 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2410,10 +2410,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); if (!(tmp = ctx->parseFileName(fileName, ctx->opaque))) goto cleanup; - if (virDomainDiskSetSource(*def, tmp) < 0) { - VIR_FREE(tmp); - goto cleanup; - } + virDomainDiskSetSource(*def, tmp); VIR_FREE(tmp); (*def)->cachemode = writeThrough ? VIR_DOMAIN_DISK_CACHE_WRITETHRU : VIR_DOMAIN_DISK_CACHE_DEFAULT; @@ -2450,19 +2447,16 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); if (!(tmp = ctx->parseFileName(fileName, ctx->opaque))) goto cleanup; - if (virDomainDiskSetSource(*def, tmp) < 0) { - VIR_FREE(tmp); - goto cleanup; - } + virDomainDiskSetSource(*def, tmp); VIR_FREE(tmp); } else if (deviceType && STRCASEEQ(deviceType, "atapi-cdrom")) { virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK); if (fileName && STRCASEEQ(fileName, "auto detect")) { - ignore_value(virDomainDiskSetSource(*def, NULL)); + virDomainDiskSetSource(*def, NULL); (*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL; - } else if (virDomainDiskSetSource(*def, fileName) < 0) { - goto cleanup; + } else { + virDomainDiskSetSource(*def, fileName); } } else if (deviceType && STRCASEEQ(deviceType, "cdrom-raw")) { /* Raw access CD-ROMs actually are device='lun' */ @@ -2470,10 +2464,10 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK); if (fileName && STRCASEEQ(fileName, "auto detect")) { - ignore_value(virDomainDiskSetSource(*def, NULL)); + virDomainDiskSetSource(*def, NULL); (*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL; - } else if (virDomainDiskSetSource(*def, fileName) < 0) { - goto cleanup; + } else { + virDomainDiskSetSource(*def, fileName); } } else if (busType == VIR_DOMAIN_DISK_BUS_SCSI && deviceType && STRCASEEQ(deviceType, "scsi-passthru")) { @@ -2481,9 +2475,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con /* SCSI-passthru CD-ROMs actually are device='lun' */ (*def)->device = VIR_DOMAIN_DISK_DEVICE_LUN; virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK); - - if (virDomainDiskSetSource(*def, fileName) < 0) - goto cleanup; + virDomainDiskSetSource(*def, fileName); } else { /* * This function was called in order to parse a CDROM device, @@ -2502,7 +2494,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con } virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); - ignore_value(virDomainDiskSetSource(*def, NULL)); + virDomainDiskSetSource(*def, NULL); } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid or not yet handled value '%s' " @@ -2514,18 +2506,14 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con } else if (device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { if (fileType != NULL && STRCASEEQ(fileType, "device")) { virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK); - if (virDomainDiskSetSource(*def, fileName) < 0) - goto cleanup; + virDomainDiskSetSource(*def, fileName); } else if (fileType != NULL && STRCASEEQ(fileType, "file")) { char *tmp = NULL; virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE); if (fileName && !(tmp = ctx->parseFileName(fileName, ctx->opaque))) goto cleanup; - if (virDomainDiskSetSource(*def, tmp) < 0) { - VIR_FREE(tmp); - goto cleanup; - } + virDomainDiskSetSource(*def, tmp); VIR_FREE(tmp); } else { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 7877974628..6a0ab5c862 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -674,8 +674,8 @@ prlsdkGetDiskInfo(vzDriverPtr driver, if (!(buf = prlsdkGetStringParamVar(PrlVmDev_GetFriendlyName, prldisk))) goto cleanup; - if (*buf != '\0' && virDomainDiskSetSource(disk, buf) < 0) - goto cleanup; + if (*buf != '\0') + virDomainDiskSetSource(disk, buf); if (prlsdkGetDiskId(prldisk, &disk->bus, &disk->dst) < 0) goto cleanup; -- 2.27.0
On a Thursday in 2020, Matt Coleman wrote:
The function only returns zero or aborts, so it might as well be void. This has the added benefit of simplifying the code that calls it.
Signed-off-by: Matt Coleman <matt@datto.com> --- src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 3 +-- src/libxl/libxl_driver.c | 6 ++---- src/libxl/xen_xl.c | 3 +-- src/libxl/xen_xm.c | 13 ++++--------- src/lxc/lxc_controller.c | 13 ++----------- src/vbox/vbox_common.c | 6 +----- src/vmx/vmx.c | 36 ++++++++++++------------------------ src/vz/vz_sdk.c | 4 ++-- 9 files changed, 26 insertions(+), 61 deletions(-)
Nice cleanup. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 11/5/20 7:23 AM, Matt Coleman wrote:
The function only returns zero or aborts, so it might as well be void. This has the added benefit of simplifying the code that calls it.
Signed-off-by: Matt Coleman <matt@datto.com> --- src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 3 +-- src/libxl/libxl_driver.c | 6 ++---- src/libxl/xen_xl.c | 3 +-- src/libxl/xen_xm.c | 13 ++++--------- src/lxc/lxc_controller.c | 13 ++----------- src/vbox/vbox_common.c | 6 +----- src/vmx/vmx.c | 36 ++++++++++++------------------------ src/vz/vz_sdk.c | 4 ++-- 9 files changed, 26 insertions(+), 61 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7696b12ef9..5c8ec19da8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2241,13 +2241,12 @@ virDomainDiskGetSource(virDomainDiskDef const *def) }
-int +void virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src) { char *tmp = g_strdup(src); g_free(def->src->path); def->src->path = tmp; - return 0; }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8a487e9de3..c164b28ea1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3040,8 +3040,7 @@ void virDomainLeaseDefFree(virDomainLeaseDefPtr def); int virDomainDiskGetType(virDomainDiskDefPtr def); void virDomainDiskSetType(virDomainDiskDefPtr def, int type); const char *virDomainDiskGetSource(virDomainDiskDef const *def); -int virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src) - G_GNUC_WARN_UNUSED_RESULT; +void virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src);
With us adopting glib more and more cleanups like these will be desirable more. Any plans on continuing? Michal
On Nov 5, 2020, at 11:26 AM, Michal Prívozník <mprivozn@redhat.com> wrote:
With us adopting glib more and more cleanups like these will be desirable more. Any plans on continuing?
I discovered this when I tried to call this function without wrapping it with ignore_value(). I hadn't taken a look for other functions that might be in a similar state. I also could go a bit further with this function's rewrite: the tmp variable isn't actually necessary any longer since g_strdup() will abort on failure. So, the function might as well be just two lines: g_free() followed by g_strdup(). After looking over the rest of domain_conf.c, I've decided to produce a V2 patchset that eliminates the tmp variable in virDomainDiskSetSource() and addresses several other similar issues with other functions in the file. -- Matt
On Nov 5, 2020, at 3:43 PM, Matt Coleman <mcoleman@datto.com> wrote:
I also could go a bit further with this function's rewrite: the tmp variable isn't actually necessary any longer since g_strdup() will abort on failure. So, the function might as well be just two lines: g_free() followed by g_strdup().
I realize that I was wrong about this because it does get called with src == def->src->path, which results in it attempting to dup already-freed memory. -- Matt
On 11/5/20 11:26 AM, Michal Prívozník wrote:
On 11/5/20 7:23 AM, Matt Coleman wrote:
The function only returns zero or aborts, so it might as well be void. This has the added benefit of simplifying the code that calls it.
Signed-off-by: Matt Coleman <matt@datto.com> --- src/conf/domain_conf.c | 3 +-- src/conf/domain_conf.h | 3 +-- src/libxl/libxl_driver.c | 6 ++---- src/libxl/xen_xl.c | 3 +-- src/libxl/xen_xm.c | 13 ++++--------- src/lxc/lxc_controller.c | 13 ++----------- src/vbox/vbox_common.c | 6 +----- src/vmx/vmx.c | 36 ++++++++++++------------------------ src/vz/vz_sdk.c | 4 ++-- 9 files changed, 26 insertions(+), 61 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7696b12ef9..5c8ec19da8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2241,13 +2241,12 @@ virDomainDiskGetSource(virDomainDiskDef const *def) } -int +void virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src) { char *tmp = g_strdup(src); g_free(def->src->path); def->src->path = tmp; - return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8a487e9de3..c164b28ea1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3040,8 +3040,7 @@ void virDomainLeaseDefFree(virDomainLeaseDefPtr def); int virDomainDiskGetType(virDomainDiskDefPtr def); void virDomainDiskSetType(virDomainDiskDefPtr def, int type); const char *virDomainDiskGetSource(virDomainDiskDef const *def); -int virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src) - G_GNUC_WARN_UNUSED_RESULT; +void virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src);
With us adopting glib more and more cleanups like these will be desirable more. Any plans on continuing?
I've done it as a part of other simplifications when possible, but there have been a few cases where a function is defined differently for different platforms, and on some platforms it just always returns an error - if we changed one of those functions to return void and there was ever a case where it ended up getting called on a platform where it should be returning an error, then the user would'nt get the notification that [whatever operation] depended on [whatever capability] that isn't supported on their platform. I think it might be better to have those functions just *not defined at all* on platforms where they're not supported, but that would mean cleaning up the code at the next higher level, possibly polluting it with #ifdefs in the middle of functions. Anyway, I'm all for doing this, just wanted to warn about these special cases.
participants (4)
-
Ján Tomko -
Laine Stump -
Matt Coleman -
Michal Prívozník