8:39 a.m.
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
8:39 a.m.
New subject: [PATCH 01/11] storage_file: Declare virStorageSourceParseRBDColonString only in one header
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(a)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
10:08 a.m.
New subject: [PATCH 01/11] storage_file: Declare virStorageSourceParseRBDColonString only in one header
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(a)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(a)redhat.com>
Jano
8:39 a.m.
New subject: [PATCH 02/11] virconf: Report an error in when virConfSetValue() fails
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(a)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
8:39 a.m.
New subject: [PATCH 03/11] xen_xl: Check for virConfSetValue() retval
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(a)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
8:39 a.m.
New subject: [PATCH 04/11] src: Declare and use g_autoptr(virConfValue)
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(a)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
8:39 a.m.
New subject: [PATCH 05/11] virconf: Make virConfSetValue() clear consumed pointer
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(a)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
8:39 a.m.
New subject: [PATCH 06/11] libxl: Don't use a static buffer in xenParseXLVnuma()
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(a)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
8:39 a.m.
New subject: [PATCH 07/11] libxl: Allocate @libxldisk in xenParseXLDisk() on stack
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(a)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
8:39 a.m.
New subject: [PATCH 08/11] xen_xl.c: Use g_autofree more
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(a)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
10:22 a.m.
New subject: [PATCH 08/11] xen_xl.c: Use g_autofree more
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(a)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")) {
8:39 a.m.
New subject: [PATCH 09/11] xen_xl.c: Use g_autoptr() for virCPUDef
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(a)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
8:39 a.m.
New subject: [PATCH 10/11] libxl: Remove needless labels
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(a)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
8:39 a.m.
New subject: [PATCH 11/11] virsh: Remove needless labels
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(a)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
10:25 a.m.
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(a)redhat.com>
Jano
1046
days inactive
1046
days old
14 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik