Couple of cleanups I've done whilst looking around our code base. Michal Prívozník (11): storage_file: Declare virStorageSourceParseRBDColonString only in one header virconf: Report an error in when virConfSetValue() fails xen_xl: Check for virConfSetValue() retval src: Declare and use g_autoptr(virConfValue) virconf: Make virConfSetValue() clear consumed pointer libxl: Don't use a static buffer in xenParseXLVnuma() libxl: Allocate @libxldisk in xenParseXLDisk() on stack xen_xl.c: Use g_autofree more xen_xl.c: Use g_autoptr() for virCPUDef libxl: Remove needless labels virsh: Remove needless labels src/libxl/xen_common.c | 62 ++--- src/libxl/xen_xl.c | 259 ++++++------------ src/libxl/xen_xm.c | 18 +- src/storage_file/storage_source.h | 5 - .../storage_source_backingstore.h | 3 +- src/util/virconf.c | 19 +- src/util/virconf.h | 3 +- tools/virsh-host.c | 20 +- tools/vsh.c | 11 +- 9 files changed, 135 insertions(+), 265 deletions(-) -- 2.34.1
The virStorageSourceParseRBDColonString() function is declared in src/storage_file/storage_source.h and src/storage_file/storage_source_backingstore.h but implemented only in the .c that corresponds to the latter header file. Therefore, drop declaration from storage_source.h as the function is not implemented in its corresponding .c file. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_xl.c | 1 + src/storage_file/storage_source.h | 5 ----- src/storage_file/storage_source_backingstore.h | 3 ++- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 7604e3d534..94268fb76d 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -30,6 +30,7 @@ #include "viralloc.h" #include "virstring.h" #include "storage_source.h" +#include "storage_source_backingstore.h" #include "xen_xl.h" #include "libxl_capabilities.h" #include "libxl_conf.h" diff --git a/src/storage_file/storage_source.h b/src/storage_file/storage_source.h index 5aaf4c356a..0ae06f4e7d 100644 --- a/src/storage_file/storage_source.h +++ b/src/storage_file/storage_source.h @@ -66,11 +66,6 @@ int virStorageSourceNewFromBacking(virStorageSource *parent, virStorageSource **backing); -int -virStorageSourceParseRBDColonString(const char *rbdstr, - virStorageSource *src) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - int virStorageSourceGetRelativeBackingPath(virStorageSource *top, virStorageSource *base, diff --git a/src/storage_file/storage_source_backingstore.h b/src/storage_file/storage_source_backingstore.h index f5ec8e9154..e7e2a58eb9 100644 --- a/src/storage_file/storage_source_backingstore.h +++ b/src/storage_file/storage_source_backingstore.h @@ -29,7 +29,8 @@ virStorageSourceParseBackingURI(virStorageSource *src, int virStorageSourceParseRBDColonString(const char *rbdstr, - virStorageSource *src); + virStorageSource *src) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virStorageSourceParseBackingColon(virStorageSource *src, -- 2.34.1
On a Friday in 2022, Michal Privoznik wrote:
The virStorageSourceParseRBDColonString() function is declared in src/storage_file/storage_source.h and src/storage_file/storage_source_backingstore.h but implemented only in the .c that corresponds to the latter header file. Therefore, drop declaration from storage_source.h as the function is not implemented in its corresponding .c file.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_xl.c | 1 + src/storage_file/storage_source.h | 5 ----- src/storage_file/storage_source_backingstore.h | 3 ++- 3 files changed, 3 insertions(+), 6 deletions(-)
Leftover from: 2d29a3a9d86b5f95a859888283e6caa98593b1d2 Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Callers of virConfSetValue() don't report any error, they just pass the error blindly. Therefore, report an error when virConfSetValue() is about to fail. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virconf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virconf.c b/src/util/virconf.c index 07ecfc7b57..29b3622791 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -1329,6 +1329,9 @@ virConfSetValue(virConf *conf, virConfEntry *prev = NULL; if (value && value->type == VIR_CONF_STRING && value->str == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("expecting a value for value of type %s"), + virConfTypeToString(VIR_CONF_STRING)); virConfFreeValue(value); return -1; } -- 2.34.1
There's one case where the return value of virConfSetValue() is not checked for and it's in xenFormatXLInputDevs() function. Let's fix that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_xl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 94268fb76d..e3ddae8827 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -1853,7 +1853,11 @@ xenFormatXLInputDevs(virConf *conf, virDomainDef *def) goto error; virConfFreeValue(usbdevices); } else { - virConfSetValue(conf, "usbdevice", usbdevices); + if (virConfSetValue(conf, "usbdevice", usbdevices) < 0) { + usbdevices = NULL; + goto error; + } + usbdevices = NULL; } } else { VIR_FREE(usbdevices); -- 2.34.1
This commit declares g_autoptr() function for virConfValue type. At the same time, it switches variable declarations to use it. Also, in a few places we might have freed a variable twice, for instance in xenFormatXLDomainNamespaceData(). This is because virConfSetValue() consumes passed pointer (@value) even in case of failure and thus any code that uses virConfSetValue() must refrain from touching @value and it must not call virConfFreeValue(). This semantic is not obvious and will be addressed in one of future commits. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_common.c | 28 +++++++++++++--------------- src/libxl/xen_xl.c | 36 +++++++++++++----------------------- src/libxl/xen_xm.c | 4 +--- src/util/virconf.h | 1 + 4 files changed, 28 insertions(+), 41 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index e68ca1cde3..f36e269aab 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -1729,7 +1729,7 @@ xenFormatNet(virConnectPtr conn, static int xenFormatPCI(virConf *conf, virDomainDef *def) { - virConfValue *pciVal = NULL; + g_autoptr(virConfValue) pciVal = NULL; int hasPCI = 0; size_t i; @@ -1794,7 +1794,6 @@ xenFormatPCI(virConf *conf, virDomainDef *def) if (ret < 0) return -1; } - VIR_FREE(pciVal); return 0; } @@ -1970,7 +1969,7 @@ xenFormatCharDev(virConf *conf, virDomainDef *def, } else { size_t j = 0; int maxport = -1, port; - virConfValue *serialVal = NULL; + g_autoptr(virConfValue) serialVal = NULL; if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -1997,7 +1996,6 @@ xenFormatCharDev(virConf *conf, virDomainDef *def, } if (xenFormatSerial(serialVal, chr) < 0) { - VIR_FREE(serialVal); return -1; } } @@ -2009,7 +2007,6 @@ xenFormatCharDev(virConf *conf, virDomainDef *def, if (ret < 0) return -1; } - VIR_FREE(serialVal); } } else { if (xenConfigSetString(conf, "serial", "none") < 0) @@ -2224,8 +2221,8 @@ xenFormatVfb(virConf *conf, virDomainDef *def) return -1; } } else { - virConfValue *vfb; - virConfValue *disp; + g_autoptr(virConfValue) vfb = NULL; + g_autoptr(virConfValue) disp = NULL; char *vfbstr = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; @@ -2259,16 +2256,19 @@ xenFormatVfb(virConf *conf, virDomainDef *def) vfbstr = virBufferContentAndReset(&buf); - vfb = g_new0(virConfValue, 1); disp = g_new0(virConfValue, 1); - - vfb->type = VIR_CONF_LIST; - vfb->list = disp; disp->type = VIR_CONF_STRING; disp->str = vfbstr; - if (virConfSetValue(conf, "vfb", vfb) < 0) + vfb = g_new0(virConfValue, 1); + vfb->type = VIR_CONF_LIST; + vfb->list = g_steal_pointer(&disp); + + if (virConfSetValue(conf, "vfb", vfb) < 0) { + vfb = NULL; return -1; + } + vfb = NULL; } } @@ -2312,7 +2312,7 @@ xenFormatVif(virConf *conf, virDomainDef *def, const char *vif_typename) { - virConfValue *netVal = NULL; + g_autoptr(virConfValue) netVal = NULL; size_t i; int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; @@ -2333,11 +2333,9 @@ xenFormatVif(virConf *conf, goto cleanup; } - VIR_FREE(netVal); return 0; cleanup: - virConfFreeValue(netVal); return -1; } diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index e3ddae8827..815c1216f7 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -1429,7 +1429,7 @@ xenFormatXLDomainVnuma(virConf *conf, virDomainDef *def) { virDomainNuma *numa = def->numa; - virConfValue *vnumaVal; + g_autoptr(virConfValue) vnumaVal = NULL; size_t i; size_t nr_nodes; @@ -1453,12 +1453,10 @@ xenFormatXLDomainVnuma(virConf *conf, if (ret < 0) return -1; } - VIR_FREE(vnumaVal); return 0; cleanup: - virConfFreeValue(vnumaVal); return -1; } @@ -1683,7 +1681,7 @@ xenFormatXLDisk(virConfValue *list, virDomainDiskDef *disk) static int xenFormatXLDomainDisks(virConf *conf, virDomainDef *def) { - virConfValue *diskVal; + g_autoptr(virConfValue) diskVal = NULL; size_t i; diskVal = g_new0(virConfValue, 1); @@ -1705,12 +1703,10 @@ xenFormatXLDomainDisks(virConf *conf, virDomainDef *def) if (ret < 0) return -1; } - VIR_FREE(diskVal); return 0; cleanup: - virConfFreeValue(diskVal); return -1; } @@ -1807,7 +1803,7 @@ xenFormatXLInputDevs(virConf *conf, virDomainDef *def) { size_t i; const char *devtype; - virConfValue *usbdevices = NULL; + g_autoptr(virConfValue) usbdevices = NULL; virConfValue *lastdev; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { @@ -1851,7 +1847,6 @@ xenFormatXLInputDevs(virConf *conf, virDomainDef *def) * only one device present */ if (xenConfigSetString(conf, "usbdevice", usbdevices->list->str) < 0) goto error; - virConfFreeValue(usbdevices); } else { if (virConfSetValue(conf, "usbdevice", usbdevices) < 0) { usbdevices = NULL; @@ -1859,14 +1854,11 @@ xenFormatXLInputDevs(virConf *conf, virDomainDef *def) } usbdevices = NULL; } - } else { - VIR_FREE(usbdevices); } } return 0; error: - virConfFreeValue(usbdevices); return -1; } @@ -1874,7 +1866,7 @@ static int xenFormatXLUSBController(virConf *conf, virDomainDef *def) { - virConfValue *usbctrlVal = NULL; + g_autoptr(virConfValue) usbctrlVal = NULL; int hasUSBCtrl = 0; size_t i; @@ -1937,12 +1929,10 @@ xenFormatXLUSBController(virConf *conf, if (ret < 0) return -1; } - VIR_FREE(usbctrlVal); return 0; error: - virConfFreeValue(usbctrlVal); return -1; } @@ -1951,7 +1941,7 @@ static int xenFormatXLUSB(virConf *conf, virDomainDef *def) { - virConfValue *usbVal = NULL; + g_autoptr(virConfValue) usbVal = NULL; int hasUSB = 0; size_t i; @@ -2001,7 +1991,6 @@ xenFormatXLUSB(virConf *conf, if (ret < 0) return -1; } - VIR_FREE(usbVal); return 0; } @@ -2050,7 +2039,7 @@ xenFormatXLChannel(virConfValue *list, virDomainChrDef *channel) static int xenFormatXLDomainChannels(virConf *conf, virDomainDef *def) { - virConfValue *channelVal = NULL; + g_autoptr(virConfValue) channelVal = NULL; size_t i; channelVal = g_new0(virConfValue, 1); @@ -2075,11 +2064,9 @@ xenFormatXLDomainChannels(virConf *conf, virDomainDef *def) goto cleanup; } - VIR_FREE(channelVal); return 0; cleanup: - virConfFreeValue(channelVal); return -1; } @@ -2087,7 +2074,7 @@ static int xenFormatXLDomainNamespaceData(virConf *conf, virDomainDef *def) { libxlDomainXmlNsDef *nsdata = def->namespaceData; - virConfValue *args = NULL; + g_autoptr(virConfValue) args = NULL; size_t i; if (!nsdata) @@ -2118,14 +2105,17 @@ xenFormatXLDomainNamespaceData(virConf *conf, virDomainDef *def) args->list = val; } - if (args->list != NULL) - if (virConfSetValue(conf, "device_model_args", args) < 0) + if (args->list != NULL) { + if (virConfSetValue(conf, "device_model_args", args) < 0) { + args = NULL; goto error; + } + args = NULL; + } return 0; error: - virConfFreeValue(args); return -1; } diff --git a/src/libxl/xen_xm.c b/src/libxl/xen_xm.c index 9c2d091918..b8f0471514 100644 --- a/src/libxl/xen_xm.c +++ b/src/libxl/xen_xm.c @@ -340,7 +340,7 @@ xenFormatXMDisk(virConfValue *list, static int xenFormatXMDisks(virConf *conf, virDomainDef *def) { - virConfValue *diskVal = NULL; + g_autoptr(virConfValue) diskVal = NULL; size_t i = 0; diskVal = g_new0(virConfValue, 1); @@ -362,12 +362,10 @@ xenFormatXMDisks(virConf *conf, virDomainDef *def) if (ret < 0) goto cleanup; } - VIR_FREE(diskVal); return 0; cleanup: - virConfFreeValue(diskVal); return -1; } diff --git a/src/util/virconf.h b/src/util/virconf.h index 937e5d43ed..558009b92e 100644 --- a/src/util/virconf.h +++ b/src/util/virconf.h @@ -78,6 +78,7 @@ virConf *virConfReadString(const char *memory, int virConfFree(virConf *conf); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virConf, virConfFree); void virConfFreeValue(virConfValue *val); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virConfValue, virConfFreeValue); virConfValue *virConfGetValue(virConf *conf, const char *setting); -- 2.34.1
The way that virConfSetValue() works (and the way it is even documented) is that the @value pointer is always consumed. However, since the first order pointer is passed it leaves callers in a pickle situation - they always have to set pointer to NULL after calling virConfSetValue() to avoid touching it. Let's switch @value to a double pointer and clear it inside the function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_common.c | 37 +++++++++----------------- src/libxl/xen_xl.c | 60 +++++++++++++----------------------------- src/libxl/xen_xm.c | 9 +++---- src/util/virconf.c | 16 +++++++---- src/util/virconf.h | 2 +- 5 files changed, 46 insertions(+), 78 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index f36e269aab..2d363ac2fb 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -259,7 +259,7 @@ xenConfigSetInt(virConf *conf, const char *setting, long long l) value->next = NULL; value->l = l; - return virConfSetValue(conf, setting, value); + return virConfSetValue(conf, setting, &value); } @@ -274,7 +274,7 @@ xenConfigSetString(virConf *conf, const char *setting, const char *str) value->next = NULL; value->str = g_strdup(str); - return virConfSetValue(conf, setting, value); + return virConfSetValue(conf, setting, &value); } @@ -1788,12 +1788,9 @@ xenFormatPCI(virConf *conf, virDomainDef *def) } } - if (pciVal->list != NULL) { - int ret = virConfSetValue(conf, "pci", pciVal); - pciVal = NULL; - if (ret < 0) - return -1; - } + if (pciVal->list != NULL && + virConfSetValue(conf, "pci", &pciVal) < 0) + return -1; return 0; } @@ -2000,13 +1997,9 @@ xenFormatCharDev(virConf *conf, virDomainDef *def, } } - if (serialVal->list != NULL) { - int ret = virConfSetValue(conf, "serial", serialVal); - - serialVal = NULL; - if (ret < 0) - return -1; - } + if (serialVal->list != NULL && + virConfSetValue(conf, "serial", &serialVal) < 0) + return -1; } } else { if (xenConfigSetString(conf, "serial", "none") < 0) @@ -2264,11 +2257,8 @@ xenFormatVfb(virConf *conf, virDomainDef *def) vfb->type = VIR_CONF_LIST; vfb->list = g_steal_pointer(&disp); - if (virConfSetValue(conf, "vfb", vfb) < 0) { - vfb = NULL; + if (virConfSetValue(conf, "vfb", &vfb) < 0) return -1; - } - vfb = NULL; } } @@ -2326,12 +2316,9 @@ xenFormatVif(virConf *conf, goto cleanup; } - if (netVal->list != NULL) { - int ret = virConfSetValue(conf, "vif", netVal); - netVal = NULL; - if (ret < 0) - goto cleanup; - } + if (netVal->list != NULL && + virConfSetValue(conf, "vif", &netVal) < 0) + goto cleanup; return 0; diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 815c1216f7..6e489f35ad 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -1447,12 +1447,9 @@ xenFormatXLDomainVnuma(virConf *conf, goto cleanup; } - if (vnumaVal->list != NULL) { - int ret = virConfSetValue(conf, "vnuma", vnumaVal); - vnumaVal = NULL; - if (ret < 0) - return -1; - } + if (vnumaVal->list != NULL && + virConfSetValue(conf, "vnuma", &vnumaVal) < 0) + return -1; return 0; @@ -1697,12 +1694,9 @@ xenFormatXLDomainDisks(virConf *conf, virDomainDef *def) goto cleanup; } - if (diskVal->list != NULL) { - int ret = virConfSetValue(conf, "disk", diskVal); - diskVal = NULL; - if (ret < 0) - return -1; - } + if (diskVal->list != NULL && + virConfSetValue(conf, "disk", &diskVal) < 0) + return -1; return 0; @@ -1848,11 +1842,8 @@ xenFormatXLInputDevs(virConf *conf, virDomainDef *def) if (xenConfigSetString(conf, "usbdevice", usbdevices->list->str) < 0) goto error; } else { - if (virConfSetValue(conf, "usbdevice", usbdevices) < 0) { - usbdevices = NULL; + if (virConfSetValue(conf, "usbdevice", &usbdevices) < 0) goto error; - } - usbdevices = NULL; } } } @@ -1923,12 +1914,9 @@ xenFormatXLUSBController(virConf *conf, } } - if (usbctrlVal->list != NULL) { - int ret = virConfSetValue(conf, "usbctrl", usbctrlVal); - usbctrlVal = NULL; - if (ret < 0) - return -1; - } + if (usbctrlVal->list != NULL && + virConfSetValue(conf, "usbctrl", &usbctrlVal) < 0) + return -1; return 0; @@ -1985,12 +1973,9 @@ xenFormatXLUSB(virConf *conf, } } - if (usbVal->list != NULL) { - int ret = virConfSetValue(conf, "usbdev", usbVal); - usbVal = NULL; - if (ret < 0) - return -1; - } + if (usbVal->list != NULL && + virConfSetValue(conf, "usbdev", &usbVal) < 0) + return -1; return 0; } @@ -2057,12 +2042,9 @@ xenFormatXLDomainChannels(virConf *conf, virDomainDef *def) goto cleanup; } - if (channelVal->list != NULL) { - int ret = virConfSetValue(conf, "channel", channelVal); - channelVal = NULL; - if (ret < 0) - goto cleanup; - } + if (channelVal->list != NULL && + virConfSetValue(conf, "channel", &channelVal) < 0) + goto cleanup; return 0; @@ -2105,13 +2087,9 @@ xenFormatXLDomainNamespaceData(virConf *conf, virDomainDef *def) args->list = val; } - if (args->list != NULL) { - if (virConfSetValue(conf, "device_model_args", args) < 0) { - args = NULL; - goto error; - } - args = NULL; - } + if (args->list != NULL && + virConfSetValue(conf, "device_model_args", &args) < 0) + goto error; return 0; diff --git a/src/libxl/xen_xm.c b/src/libxl/xen_xm.c index b8f0471514..f20307430c 100644 --- a/src/libxl/xen_xm.c +++ b/src/libxl/xen_xm.c @@ -356,12 +356,9 @@ xenFormatXMDisks(virConf *conf, virDomainDef *def) goto cleanup; } - if (diskVal->list != NULL) { - int ret = virConfSetValue(conf, "disk", diskVal); - diskVal = NULL; - if (ret < 0) - goto cleanup; - } + if (diskVal->list != NULL && + virConfSetValue(conf, "disk", &diskVal) < 0) + goto cleanup; return 0; diff --git a/src/util/virconf.c b/src/util/virconf.c index 29b3622791..cd45c5f657 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -1323,16 +1323,22 @@ int virConfGetValueULLong(virConf *conf, int virConfSetValue(virConf *conf, const char *setting, - virConfValue *value) + virConfValue **value) { virConfEntry *cur; virConfEntry *prev = NULL; - if (value && value->type == VIR_CONF_STRING && value->str == NULL) { + if (!value) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of conf API")); + return -1; + } + + if (*value && (*value)->type == VIR_CONF_STRING && !(*value)->str) { virReportError(VIR_ERR_INTERNAL_ERROR, _("expecting a value for value of type %s"), virConfTypeToString(VIR_CONF_STRING)); - virConfFreeValue(value); + g_clear_pointer(value, virConfFreeValue); return -1; } @@ -1348,7 +1354,7 @@ virConfSetValue(virConf *conf, cur = g_new0(virConfEntry, 1); cur->comment = NULL; cur->name = g_strdup(setting); - cur->value = value; + cur->value = g_steal_pointer(value); if (prev) { cur->next = prev->next; prev->next = cur; @@ -1358,7 +1364,7 @@ virConfSetValue(virConf *conf, } } else { virConfFreeValue(cur->value); - cur->value = value; + cur->value = g_steal_pointer(value); } return 0; } diff --git a/src/util/virconf.h b/src/util/virconf.h index 558009b92e..e656a6a815 100644 --- a/src/util/virconf.h +++ b/src/util/virconf.h @@ -115,7 +115,7 @@ int virConfGetValueULLong(virConf *conf, int virConfSetValue(virConf *conf, const char *setting, - virConfValue *value); + virConfValue **value); int virConfWalk(virConf *conf, virConfWalkCallback callback, void *opaque); -- 2.34.1
The xenParseXLVnuma() function is responsible for parsing 'vnuma' part of XL config and setting corresponding values in virDomainDef. While doing so it uses a static buffer which is set to data we are interested in and then parsing the buffer further (e.g. string to integer conversion, bitmap parsing, and so on). Well, the data we are interested in are already in a string (@data) which can be used directly rendering this intermediary buffer needless. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_xl.c | 43 ++++--------------------------------------- 1 file changed, 4 insertions(+), 39 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 6e489f35ad..3a4e21ee0e 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -394,7 +394,6 @@ xenParseXLVnuma(virConf *conf, virDomainDef *def) { int ret = -1; - char *tmp = NULL; size_t vcpus = 0; size_t nr_nodes = 0; size_t vnodeCnt = 0; @@ -446,19 +445,10 @@ xenParseXLVnuma(virConf *conf, data++; if (*data) { - char vtoken[64]; - if (STRPREFIX(str, "pnode")) { unsigned int cellid; - if (virStrcpyStatic(vtoken, data) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("vnuma vnode %zu pnode '%s' too long for destination"), - vnodeCnt, data); - goto cleanup; - } - - if ((virStrToLong_ui(vtoken, NULL, 10, &cellid) < 0) || + if ((virStrToLong_ui(data, NULL, 10, &cellid) < 0) || (cellid >= nr_nodes)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("vnuma vnode %zu contains invalid pnode value '%s'"), @@ -467,27 +457,13 @@ xenParseXLVnuma(virConf *conf, } pnode = cellid; } else if (STRPREFIX(str, "size")) { - if (virStrcpyStatic(vtoken, data) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("vnuma vnode %zu size '%s' too long for destination"), - vnodeCnt, data); - goto cleanup; - } - - if (virStrToLong_ull(vtoken, NULL, 10, &kbsize) < 0) + if (virStrToLong_ull(data, NULL, 10, &kbsize) < 0) goto cleanup; virDomainNumaSetNodeMemorySize(numa, vnodeCnt, (kbsize * 1024)); } else if (STRPREFIX(str, "vcpus")) { - if (virStrcpyStatic(vtoken, data) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("vnuma vnode %zu vcpus '%s' too long for destination"), - vnodeCnt, data); - goto cleanup; - } - - if (virBitmapParse(vtoken, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) + if (virBitmapParse(data, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) goto cleanup; virDomainNumaSetNodeCpumask(numa, vnodeCnt, cpumask); @@ -498,17 +474,7 @@ xenParseXLVnuma(virConf *conf, size_t i, ndistances; unsigned int value; - if (virStrcpyStatic(vtoken, data) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("vnuma vnode %zu vdistances '%s' too long for destination"), - vnodeCnt, data); - goto cleanup; - } - - VIR_FREE(tmp); - tmp = g_strdup(vtoken); - - if (!(token = g_strsplit(tmp, ",", 0))) + if (!(token = g_strsplit(data, ",", 0))) goto cleanup; ndistances = g_strv_length(token); @@ -571,7 +537,6 @@ xenParseXLVnuma(virConf *conf, cleanup: if (ret) VIR_FREE(cpu); - VIR_FREE(tmp); return ret; } -- 2.34.1
In xenParseXLDisk() the @libxldisk variable (which is type of libxl_device_disk) is allocated on heap. But this is not necessary as nothing in the function needs that approach. Allocate the variable on the stack and drop corresponding VIR_FREE() call. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_xl.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 3a4e21ee0e..f32a6cd65a 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -641,11 +641,9 @@ xenParseXLDisk(virConf *conf, virDomainDef *def) int ret = -1; virConfValue *list = virConfGetValue(conf, "disk"); XLU_Config *xluconf; - libxl_device_disk *libxldisk; + libxl_device_disk libxldisk; virDomainDiskDef *disk = NULL; - libxldisk = g_new0(libxl_device_disk, 1); - if (!(xluconf = xlu_cfg_init(stderr, "command line"))) goto cleanup; @@ -657,23 +655,23 @@ xenParseXLDisk(virConf *conf, virDomainDef *def) if (list->type != VIR_CONF_STRING || list->str == NULL) goto skipdisk; - libxl_device_disk_init(libxldisk); + libxl_device_disk_init(&libxldisk); - if (xlu_disk_parse(xluconf, 1, &disk_spec, libxldisk)) + if (xlu_disk_parse(xluconf, 1, &disk_spec, &libxldisk)) goto fail; if (!(disk = virDomainDiskDefNew(NULL))) goto fail; - if (xenParseXLDiskSrc(disk, libxldisk->pdev_path) < 0) + if (xenParseXLDiskSrc(disk, libxldisk.pdev_path) < 0) goto fail; - disk->dst = g_strdup(libxldisk->vdev); + disk->dst = g_strdup(libxldisk.vdev); - disk->src->readonly = !libxldisk->readwrite; - disk->removable = libxldisk->removable; + disk->src->readonly = !libxldisk.readwrite; + disk->removable = libxldisk.removable; - if (libxldisk->is_cdrom) { + if (libxldisk.is_cdrom) { virDomainDiskSetDriver(disk, "qemu"); virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE); @@ -683,7 +681,7 @@ xenParseXLDisk(virConf *conf, virDomainDef *def) else disk->src->format = VIR_STORAGE_FILE_RAW; } else { - switch (libxldisk->format) { + switch (libxldisk.format) { case LIBXL_DISK_FORMAT_QCOW: disk->src->format = VIR_STORAGE_FILE_QCOW; break; @@ -711,11 +709,11 @@ xenParseXLDisk(virConf *conf, virDomainDef *def) default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk image format not supported: %s"), - libxl_disk_format_to_string(libxldisk->format)); + libxl_disk_format_to_string(libxldisk.format)); goto fail; } - switch (libxldisk->backend) { + switch (libxldisk.backend) { case LIBXL_DISK_BACKEND_QDISK: case LIBXL_DISK_BACKEND_UNKNOWN: virDomainDiskSetDriver(disk, "qemu"); @@ -735,22 +733,22 @@ xenParseXLDisk(virConf *conf, virDomainDef *def) default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk backend not supported: %s"), - libxl_disk_backend_to_string(libxldisk->backend)); + libxl_disk_backend_to_string(libxldisk.backend)); goto fail; } } - if (STRPREFIX(libxldisk->vdev, "xvd") || + if (STRPREFIX(libxldisk.vdev, "xvd") || def->os.type != VIR_DOMAIN_OSTYPE_HVM) disk->bus = VIR_DOMAIN_DISK_BUS_XEN; - else if (STRPREFIX(libxldisk->vdev, "sd")) + else if (STRPREFIX(libxldisk.vdev, "sd")) disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; else disk->bus = VIR_DOMAIN_DISK_BUS_IDE; VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk); - libxl_device_disk_dispose(libxldisk); + libxl_device_disk_dispose(&libxldisk); skipdisk: list = list->next; @@ -761,11 +759,10 @@ xenParseXLDisk(virConf *conf, virDomainDef *def) cleanup: virDomainDiskDefFree(disk); xlu_cfg_destroy(xluconf); - VIR_FREE(libxldisk); return ret; fail: - libxl_device_disk_dispose(libxldisk); + libxl_device_disk_dispose(&libxldisk); goto cleanup; } -- 2.34.1
There are few places inside src/libxl/xen_xl.c that can benefit from g_autofree. Let them use automatic memory freeing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_xl.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index f32a6cd65a..043f3c27db 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -321,10 +321,11 @@ xenParseXLSpice(virConf *conf, virDomainDef *def) { virDomainGraphicsDef *graphics = NULL; unsigned long port; - char *listenAddr = NULL; int val; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { + g_autofree char *listenAddr = NULL; + if (xenConfigGetBool(conf, "spice", &val, 0) < 0) return -1; @@ -335,7 +336,6 @@ xenParseXLSpice(virConf *conf, virDomainDef *def) goto cleanup; if (virDomainGraphicsListenAppendAddress(graphics, listenAddr) < 0) goto cleanup; - VIR_FREE(listenAddr); if (xenConfigGetULong(conf, "spicetls_port", &port, 0) < 0) goto cleanup; @@ -384,7 +384,6 @@ xenParseXLSpice(virConf *conf, virDomainDef *def) return 0; cleanup: - VIR_FREE(listenAddr); virDomainGraphicsDefFree(graphics); return -1; } @@ -575,7 +574,6 @@ xenParseXLXenbusLimits(virConf *conf, virDomainDef *def) static int xenParseXLDiskSrc(virDomainDiskDef *disk, char *srcstr) { - char *tmpstr = NULL; int ret = -1; /* A NULL source is valid, e.g. an empty CDROM */ @@ -583,6 +581,8 @@ xenParseXLDiskSrc(virDomainDiskDef *disk, char *srcstr) return 0; if (STRPREFIX(srcstr, "rbd:")) { + g_autofree char *tmpstr = NULL; + if (!(tmpstr = virStringReplace(srcstr, "\\\\", "\\"))) goto cleanup; @@ -596,7 +596,6 @@ xenParseXLDiskSrc(virDomainDiskDef *disk, char *srcstr) } cleanup: - VIR_FREE(tmpstr); return ret; } @@ -952,13 +951,13 @@ xenParseXLChannel(virConf *conf, virDomainDef *def) { virConfValue *list = virConfGetValue(conf, "channel"); virDomainChrDef *channel = NULL; - char *name = NULL; - char *path = NULL; if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { g_autofree char *type = NULL; + g_autofree char *name = NULL; + g_autofree char *path = NULL; char *key; if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) @@ -1003,7 +1002,6 @@ xenParseXLChannel(virConf *conf, virDomainDef *def) channel->source->data.nix.path = g_steal_pointer(&path); } else if (STRPREFIX(type, "pty")) { channel->source->type = VIR_DOMAIN_CHR_TYPE_PTY; - VIR_FREE(path); } else { goto cleanup; } @@ -1023,8 +1021,6 @@ xenParseXLChannel(virConf *conf, virDomainDef *def) cleanup: virDomainChrDefFree(channel); - VIR_FREE(path); - VIR_FREE(name); return -1; } @@ -1382,7 +1378,6 @@ xenFormatXLVnuma(virConfValue *list, list->list = numaVnode; ret = 0; - VIR_FREE(nodeVcpus); return ret; } -- 2.34.1
On a Friday in 2022, Michal Privoznik wrote:
There are few places inside src/libxl/xen_xl.c that can benefit from g_autofree. Let them use automatic memory freeing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_xl.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
[...]
@@ -952,13 +951,13 @@ xenParseXLChannel(virConf *conf, virDomainDef *def) { virConfValue *list = virConfGetValue(conf, "channel"); virDomainChrDef *channel = NULL; - char *name = NULL; - char *path = NULL;
if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { g_autofree char *type = NULL; + g_autofree char *name = NULL; + g_autofree char *path = NULL; char *key;
if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) @@ -1003,7 +1002,6 @@ xenParseXLChannel(virConf *conf, virDomainDef *def)
There's a mix of g_auto with manual freeing here: } else if (STRPREFIX(key, "name=")) { int len = nextkey ? (nextkey - data) : strlen(data); VIR_FREE(name); name = g_strndup(data, len); } else if (STRPREFIX(key, "path=")) { int len = nextkey ? (nextkey - data) : strlen(data); VIR_FREE(path); path = g_strndup(data, len); } I would prefer either altering the code to take the first key into account instead of the last one by checking: else if (!name && STRPREFIX... similarly how we do in node-based XML parsers, or leaving these two variables unconverted and leave the refactor for later. Jano
channel->source->data.nix.path = g_steal_pointer(&path); } else if (STRPREFIX(type, "pty")) {
In xenParseXLVnuma() the @cpu variable is freed explicitly. However, when switched to g_autoptr(virCPUDef) the explicit call can be removed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_xl.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 043f3c27db..25bf1f7893 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -396,7 +396,7 @@ xenParseXLVnuma(virConf *conf, size_t vcpus = 0; size_t nr_nodes = 0; size_t vnodeCnt = 0; - virCPUDef *cpu = NULL; + g_autoptr(virCPUDef) cpu = NULL; virConfValue *list; virConfValue *vnode; virDomainNuma *numa; @@ -529,14 +529,11 @@ xenParseXLVnuma(virConf *conf, } cpu->type = VIR_CPU_TYPE_GUEST; - def->cpu = cpu; + def->cpu = g_steal_pointer(&cpu); ret = 0; cleanup: - if (ret) - VIR_FREE(cpu); - return ret; } -- 2.34.1
After previous cleanups some labels are needless: they contain nothing but a return statement. Drop such labels and return directly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/xen_common.c | 7 ++-- src/libxl/xen_xl.c | 80 ++++++++++++++---------------------------- src/libxl/xen_xm.c | 7 ++-- 3 files changed, 31 insertions(+), 63 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 2d363ac2fb..c3fa98b71d 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -2313,17 +2313,14 @@ xenFormatVif(virConf *conf, for (i = 0; i < def->nnets; i++) { if (xenFormatNet(conn, netVal, def->nets[i], hvm, vif_typename) < 0) - goto cleanup; + return -1; } if (netVal->list != NULL && virConfSetValue(conf, "vif", &netVal) < 0) - goto cleanup; + return -1; return 0; - - cleanup: - return -1; } diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 25bf1f7893..e3a87890af 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -392,7 +392,6 @@ static int xenParseXLVnuma(virConf *conf, virDomainDef *def) { - int ret = -1; size_t vcpus = 0; size_t nr_nodes = 0; size_t vnodeCnt = 0; @@ -416,7 +415,7 @@ xenParseXLVnuma(virConf *conf, } if (!virDomainNumaSetNodeCount(numa, nr_nodes)) - goto cleanup; + return -1; cpu = virCPUDefNew(); @@ -439,7 +438,7 @@ xenParseXLVnuma(virConf *conf, virReportError(VIR_ERR_INTERNAL_ERROR, _("vnuma vnode invalid format '%s'"), str); - goto cleanup; + return -1; } data++; @@ -452,18 +451,18 @@ xenParseXLVnuma(virConf *conf, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("vnuma vnode %zu contains invalid pnode value '%s'"), vnodeCnt, data); - goto cleanup; + return -1; } pnode = cellid; } else if (STRPREFIX(str, "size")) { if (virStrToLong_ull(data, NULL, 10, &kbsize) < 0) - goto cleanup; + return -1; virDomainNumaSetNodeMemorySize(numa, vnodeCnt, (kbsize * 1024)); } else if (STRPREFIX(str, "vcpus")) { if (virBitmapParse(data, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto cleanup; + return -1; virDomainNumaSetNodeCpumask(numa, vnodeCnt, cpumask); vcpus += virBitmapCountBits(cpumask); @@ -474,7 +473,7 @@ xenParseXLVnuma(virConf *conf, unsigned int value; if (!(token = g_strsplit(data, ",", 0))) - goto cleanup; + return -1; ndistances = g_strv_length(token); @@ -482,23 +481,23 @@ xenParseXLVnuma(virConf *conf, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("vnuma pnode %d configured '%s' (count %zu) doesn't fit the number of specified vnodes %zu"), pnode, str, ndistances, nr_nodes); - goto cleanup; + return -1; } if (virDomainNumaSetNodeDistanceCount(numa, vnodeCnt, ndistances) != ndistances) - goto cleanup; + return -1; for (i = 0; i < ndistances; i++) { if ((virStrToLong_ui(token[i], NULL, 10, &value) < 0) || (virDomainNumaSetNodeDistance(numa, vnodeCnt, i, value) != value)) - goto cleanup; + return -1; } } else { virReportError(VIR_ERR_CONF_SYNTAX, _("Invalid vnuma configuration for vnode %zu"), vnodeCnt); - goto cleanup; + return -1; } } vnode = vnode->next; @@ -511,7 +510,7 @@ xenParseXLVnuma(virConf *conf, virReportError(VIR_ERR_CONF_SYNTAX, _("Incomplete vnuma configuration for vnode %zu"), vnodeCnt); - goto cleanup; + return -1; } list = list->next; @@ -525,16 +524,13 @@ xenParseXLVnuma(virConf *conf, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("vnuma configuration contains %zu vcpus, which is greater than %zu maxvcpus"), vcpus, def->maxvcpus); - goto cleanup; + return -1; } cpu->type = VIR_CPU_TYPE_GUEST; def->cpu = g_steal_pointer(&cpu); - ret = 0; - - cleanup: - return ret; + return 0; } static int @@ -571,8 +567,6 @@ xenParseXLXenbusLimits(virConf *conf, virDomainDef *def) static int xenParseXLDiskSrc(virDomainDiskDef *disk, char *srcstr) { - int ret = -1; - /* A NULL source is valid, e.g. an empty CDROM */ if (srcstr == NULL) return 0; @@ -581,19 +575,16 @@ xenParseXLDiskSrc(virDomainDiskDef *disk, char *srcstr) g_autofree char *tmpstr = NULL; if (!(tmpstr = virStringReplace(srcstr, "\\\\", "\\"))) - goto cleanup; + return -1; virDomainDiskSetType(disk, VIR_STORAGE_TYPE_NETWORK); disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_RBD; - ret = virStorageSourceParseRBDColonString(tmpstr, disk->src); - } else { - virDomainDiskSetSource(disk, srcstr); - - ret = 0; + return virStorageSourceParseRBDColonString(tmpstr, disk->src); } - cleanup: - return ret; + virDomainDiskSetSource(disk, srcstr); + + return 0; } @@ -1398,7 +1389,7 @@ xenFormatXLDomainVnuma(virConf *conf, nr_nodes = virDomainNumaGetNodeCount(numa); for (i = 0; i < nr_nodes; i++) { if (xenFormatXLVnuma(vnumaVal, numa, i, nr_nodes) < 0) - goto cleanup; + return -1; } if (vnumaVal->list != NULL && @@ -1406,9 +1397,6 @@ xenFormatXLDomainVnuma(virConf *conf, return -1; return 0; - - cleanup: - return -1; } static int @@ -1645,7 +1633,7 @@ xenFormatXLDomainDisks(virConf *conf, virDomainDef *def) continue; if (xenFormatXLDisk(diskVal, def->disks[i]) < 0) - goto cleanup; + return -1; } if (diskVal->list != NULL && @@ -1653,9 +1641,6 @@ xenFormatXLDomainDisks(virConf *conf, virDomainDef *def) return -1; return 0; - - cleanup: - return -1; } @@ -1762,7 +1747,7 @@ xenFormatXLInputDevs(virConf *conf, virDomainDef *def) for (i = 0; i < def->ninputs; i++) { if (def->inputs[i]->bus == VIR_DOMAIN_INPUT_BUS_USB) { if (xenConfigSetInt(conf, "usb", 1) < 0) - goto error; + return -1; switch (def->inputs[i]->type) { case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1794,17 +1779,15 @@ xenFormatXLInputDevs(virConf *conf, virDomainDef *def) /* for compatibility with Xen <= 4.2, use old syntax when * only one device present */ if (xenConfigSetString(conf, "usbdevice", usbdevices->list->str) < 0) - goto error; + return -1; } else { if (virConfSetValue(conf, "usbdevice", &usbdevices) < 0) - goto error; + return -1; } } } return 0; - error: - return -1; } static int @@ -1847,7 +1830,7 @@ xenFormatXLUSBController(virConf *conf, break; default: - goto error; + return -1; } } @@ -1873,9 +1856,6 @@ xenFormatXLUSBController(virConf *conf, return -1; return 0; - - error: - return -1; } @@ -1993,17 +1973,14 @@ xenFormatXLDomainChannels(virConf *conf, virDomainDef *def) continue; if (xenFormatXLChannel(channelVal, def->channels[i]) < 0) - goto cleanup; + return -1; } if (channelVal->list != NULL && virConfSetValue(conf, "channel", &channelVal) < 0) - goto cleanup; + return -1; return 0; - - cleanup: - return -1; } static int @@ -2043,12 +2020,9 @@ xenFormatXLDomainNamespaceData(virConf *conf, virDomainDef *def) if (args->list != NULL && virConfSetValue(conf, "device_model_args", &args) < 0) - goto error; + return -1; return 0; - - error: - return -1; } virConf * diff --git a/src/libxl/xen_xm.c b/src/libxl/xen_xm.c index f20307430c..2e636d874e 100644 --- a/src/libxl/xen_xm.c +++ b/src/libxl/xen_xm.c @@ -353,17 +353,14 @@ xenFormatXMDisks(virConf *conf, virDomainDef *def) continue; if (xenFormatXMDisk(diskVal, def->disks[i]) < 0) - goto cleanup; + return -1; } if (diskVal->list != NULL && virConfSetValue(conf, "disk", &diskVal) < 0) - goto cleanup; + return -1; return 0; - - cleanup: - return -1; } -- 2.34.1
There are few places where a cleanup label contains nothing but a return statement. Drop such labels and return directly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-host.c | 20 ++++++-------------- tools/vsh.c | 11 ++++------- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 5ee3834de2..2e3cbc39b6 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1195,7 +1195,6 @@ static bool cmdCPUCompare(vshControl *ctl, const vshCmd *cmd) { const char *from = NULL; - bool ret = false; int result; g_auto(GStrv) cpus = NULL; unsigned int flags = 0; @@ -1219,7 +1218,7 @@ cmdCPUCompare(vshControl *ctl, const vshCmd *cmd) case VIR_CPU_COMPARE_INCOMPATIBLE: vshPrint(ctl, _("CPU described in %s is incompatible with host CPU\n"), from); - goto cleanup; + return false; break; case VIR_CPU_COMPARE_IDENTICAL: @@ -1235,13 +1234,10 @@ cmdCPUCompare(vshControl *ctl, const vshCmd *cmd) case VIR_CPU_COMPARE_ERROR: default: vshError(ctl, _("Failed to compare host CPU with %s"), from); - goto cleanup; + return false; } - ret = true; - - cleanup: - return ret; + return true; } /* @@ -1615,7 +1611,6 @@ cmdHypervisorCPUCompare(vshControl *ctl, const char *emulator = NULL; const char *arch = NULL; const char *machine = NULL; - bool ret = false; int result; g_auto(GStrv) cpus = NULL; unsigned int flags = 0; @@ -1646,7 +1641,7 @@ cmdHypervisorCPUCompare(vshControl *ctl, _("CPU described in %s is incompatible with the CPU provided " "by hypervisor on the host\n"), from); - goto cleanup; + return false; break; case VIR_CPU_COMPARE_IDENTICAL: @@ -1666,13 +1661,10 @@ cmdHypervisorCPUCompare(vshControl *ctl, case VIR_CPU_COMPARE_ERROR: default: vshError(ctl, _("Failed to compare hypervisor CPU with %s"), from); - goto cleanup; + return false; } - ret = true; - - cleanup: - return ret; + return true; } diff --git a/tools/vsh.c b/tools/vsh.c index e3e27a0ba6..c0098054e0 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3360,7 +3360,6 @@ const vshCmdInfo info_complete[] = { bool cmdComplete(vshControl *ctl, const vshCmd *cmd) { - bool ret = false; const vshClientHooks *hooks = ctl->hooks; int stdin_fileno = STDIN_FILENO; const char *arg = ""; @@ -3370,7 +3369,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; if (vshCommandOptStringQuiet(ctl, cmd, "string", &arg) <= 0) - goto cleanup; + return false; /* This command is flagged VSH_CMD_FLAG_NOCONNECT because we * need to prevent auth hooks reading any input. Therefore, we @@ -3378,7 +3377,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) VIR_FORCE_CLOSE(stdin_fileno); if (!(hooks && hooks->connHandler && hooks->connHandler(ctl))) - goto cleanup; + return false; while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { if (virBufferUse(&buf) != 0) @@ -3397,7 +3396,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) rl_point = strlen(rl_line_buffer); if (!(matches = vshReadlineCompletion(arg, 0, 0))) - goto cleanup; + return false; for (iter = matches; *iter; iter++) { if (iter == matches && matches[1]) @@ -3405,9 +3404,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) printf("%s\n", *iter); } - ret = true; - cleanup: - return ret; + return true; } -- 2.34.1
On a Friday in 2022, Michal Privoznik wrote:
Couple of cleanups I've done whilst looking around our code base.
Michal Prívozník (11): storage_file: Declare virStorageSourceParseRBDColonString only in one header virconf: Report an error in when virConfSetValue() fails xen_xl: Check for virConfSetValue() retval src: Declare and use g_autoptr(virConfValue) virconf: Make virConfSetValue() clear consumed pointer libxl: Don't use a static buffer in xenParseXLVnuma() libxl: Allocate @libxldisk in xenParseXLDisk() on stack xen_xl.c: Use g_autofree more xen_xl.c: Use g_autoptr() for virCPUDef libxl: Remove needless labels virsh: Remove needless labels
src/libxl/xen_common.c | 62 ++--- src/libxl/xen_xl.c | 259 ++++++------------ src/libxl/xen_xm.c | 18 +- src/storage_file/storage_source.h | 5 - .../storage_source_backingstore.h | 3 +- src/util/virconf.c | 19 +- src/util/virconf.h | 3 +- tools/virsh-host.c | 20 +- tools/vsh.c | 11 +- 9 files changed, 135 insertions(+), 265 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik