8:28 a.m.
This patchset finishes the conversion to virConfGetValue* functions,
started by Daniel Berrange a few months ago.
Please, mind that although we could make virConfGetValue* functions more
generic in order to support numbers and booleans as strings, that
doesn't seem the safest path to take. The side-effect of this is that we
will have to live with some specific code doing that as part of vmx and
xen_common.
Once this patchset gets merged,
https://wiki.libvirt.org/page/BiteSizedTasks#Finish_conversion_to_virConf...
can be removed.
- Changes since v1:
All the "values" from virConfGetValueString() are freed
- Changes since v2:
All comments from Ján Tomko have been addressed;
A few leaks were (possibly) found and they're addressed in the last
patch of the series;
- Changes since v3:
All comments from Ján Tomko have been addressed in order to lower the
non-whitespace changes in the first patch;
- Changes since v4:
All comments from Ján Tomko have been addressed (hopefully, they
actually were this time :-));
- Changes since v5:
All comments from John Ferlan have been addressed, including splitting
the patches per "function" touched. Some new patches were also needed
due to a behaviour change introduced by the "xen_common: Change
xenParsePCIList to using virConfGetValueStringList" patch.
- Changes since v6:
Actually properly building the code /o\ (John, --with-libxl is what
you need!)
Squashed all the patches fixing the leak with the patch that
introduces the leak
Followed John's suggestions and comments in order to try to keep the
new checks as close as possible to the old ones.
Fabiano Fidêncio (6):
xen_common: Change xenConfigCopyStringInternal to using
virConfGetValueString
xen_common: Change xenConfigGetUUID to using virConfGetValueString
xen_common: Change xenConfigGetString to using virConfGetValueString
xen_common: Change xenParsePCIList to using virConfGetValueStringList
xen_common: Change xenParseVfbs to using virConfGetValueStringList
xen_common: Change xenParseCharDev to using virConfGetValueStringList
src/xenconfig/xen_common.c | 202 ++++++++++++++++++++-----------------
src/xenconfig/xen_common.h | 2 +-
src/xenconfig/xen_xl.c | 10 +-
src/xenconfig/xen_xm.c | 7 +-
4 files changed, 120 insertions(+), 101 deletions(-)
--
2.17.1
8:28 a.m.
New subject: [libvirt] [libvirt PATCH v7 1/6] xen_common: Change xenConfigCopyStringInternal to using virConfGetValueString
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
---
src/xenconfig/xen_common.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 36a9d27c80..a35e1aff58 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -145,23 +145,13 @@ xenConfigCopyStringInternal(virConfPtr conf,
char **value,
int allowMissing)
{
- virConfValuePtr val;
+ int rc;
*value = NULL;
- if (!(val = virConfGetValue(conf, name))) {
- if (allowMissing)
- return 0;
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("config value %s was missing"), name);
+ if ((rc = virConfGetValueString(conf, name, value)) < 0)
return -1;
- }
- if (val->type != VIR_CONF_STRING) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("config value %s was not a string"), name);
- return -1;
- }
- if (!val->str) {
+ if (rc == 0) {
if (allowMissing)
return 0;
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -169,7 +159,7 @@ xenConfigCopyStringInternal(virConfPtr conf,
return -1;
}
- return VIR_STRDUP(*value, val->str);
+ return 1;
}
--
2.17.1
8:28 a.m.
New subject: [libvirt] [libvirt PATCH v7 2/6] xen_common: Change xenConfigGetUUID to using virConfGetValueString
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
---
src/xenconfig/xen_common.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index a35e1aff58..587bab2b19 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -183,7 +183,8 @@ xenConfigCopyStringOpt(virConfPtr conf, const char *name, char
**value)
static int
xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid)
{
- virConfValuePtr val;
+ VIR_AUTOFREE(char *) string = NULL;
+ int rc;
if (!uuid || !name || !conf) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -191,7 +192,11 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char
*uuid)
return -1;
}
- if (!(val = virConfGetValue(conf, name))) {
+
+ if ((rc = virConfGetValueString(conf, name, &string)) < 0)
+ return -1;
+
+ if (rc == 0) {
if (virUUIDGenerate(uuid) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("Failed to generate UUID"));
@@ -201,21 +206,15 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char
*uuid)
}
}
- if (val->type != VIR_CONF_STRING) {
- virReportError(VIR_ERR_CONF_SYNTAX,
- _("config value %s not a string"), name);
- return -1;
- }
-
- if (!val->str) {
+ if (!string) {
virReportError(VIR_ERR_CONF_SYNTAX,
_("%s can't be empty"), name);
return -1;
}
- if (virUUIDParse(val->str, uuid) < 0) {
+ if (virUUIDParse(string, uuid) < 0) {
virReportError(VIR_ERR_CONF_SYNTAX,
- _("%s not parseable"), val->str);
+ _("%s not parseable"), string);
return -1;
}
--
2.17.1
8:28 a.m.
New subject: [libvirt] [libvirt PATCH v7 3/6] xen_common: Change xenConfigGetString to using virConfGetValueString
This change actually changes the behaviour of xenConfigGetString() as
now it returns a newly-allocated string.
Unfortunately, there's not much that can be done in order to avoid that
and all the callers have to be changed in order to avoid leaking the
return value.
Also, as a side-effect of the change above, the function now takes a
"char **" argument instead of a "const char **" one.
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
---
src/xenconfig/xen_common.c | 84 ++++++++++++++++++++------------------
src/xenconfig/xen_common.h | 2 +-
src/xenconfig/xen_xl.c | 10 +++--
src/xenconfig/xen_xm.c | 7 ++--
4 files changed, 56 insertions(+), 47 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 587bab2b19..7b3e5c3b44 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -228,26 +228,28 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char
*uuid)
int
xenConfigGetString(virConfPtr conf,
const char *name,
- const char **value,
+ char **value,
const char *def)
{
- virConfValuePtr val;
+ char *string = NULL;
+ int rc;
*value = NULL;
- if (!(val = virConfGetValue(conf, name))) {
- *value = def;
+ if ((rc = virConfGetValueString(conf, name, &string)) < 0)
+ return -1;
+
+ if (rc == 0) {
+ if (VIR_STRDUP(*value, def) < 0)
+ return -1;
return 0;
}
- if (val->type != VIR_CONF_STRING) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("config value %s was malformed"), name);
- return -1;
+ if (!string) {
+ if (VIR_STRDUP(*value, def) < 0)
+ return -1;
+ } else {
+ *value = string;
}
- if (!val->str)
- *value = def;
- else
- *value = val->str;
return 0;
}
@@ -345,32 +347,34 @@ xenParseTimeOffset(virConfPtr conf, virDomainDefPtr def)
static int
xenParseEventsActions(virConfPtr conf, virDomainDefPtr def)
{
- const char *str = NULL;
+ VIR_AUTOFREE(char *) on_poweroff = NULL;
+ VIR_AUTOFREE(char *) on_reboot = NULL;
+ VIR_AUTOFREE(char *) on_crash = NULL;
- if (xenConfigGetString(conf, "on_poweroff", &str, "destroy")
< 0)
+ if (xenConfigGetString(conf, "on_poweroff", &on_poweroff,
"destroy") < 0)
return -1;
- if ((def->onPoweroff = virDomainLifecycleActionTypeFromString(str)) < 0) {
+ if ((def->onPoweroff = virDomainLifecycleActionTypeFromString(on_poweroff)) <
0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected value %s for on_poweroff"), str);
+ _("unexpected value %s for on_poweroff"), on_poweroff);
return -1;
}
- if (xenConfigGetString(conf, "on_reboot", &str, "restart")
< 0)
+ if (xenConfigGetString(conf, "on_reboot", &on_reboot,
"restart") < 0)
return -1;
- if ((def->onReboot = virDomainLifecycleActionTypeFromString(str)) < 0) {
+ if ((def->onReboot = virDomainLifecycleActionTypeFromString(on_reboot)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected value %s for on_reboot"), str);
+ _("unexpected value %s for on_reboot"), on_reboot);
return -1;
}
- if (xenConfigGetString(conf, "on_crash", &str, "restart")
< 0)
+ if (xenConfigGetString(conf, "on_crash", &on_crash,
"restart") < 0)
return -1;
- if ((def->onCrash = virDomainLifecycleActionTypeFromString(str)) < 0) {
+ if ((def->onCrash = virDomainLifecycleActionTypeFromString(on_crash)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected value %s for on_crash"), str);
+ _("unexpected value %s for on_crash"), on_crash);
return -1;
}
@@ -488,7 +492,8 @@ xenParseCPUFeatures(virConfPtr conf,
virDomainXMLOptionPtr xmlopt)
{
unsigned long count = 0;
- const char *str = NULL;
+ VIR_AUTOFREE(char *) cpus = NULL;
+ VIR_AUTOFREE(char *) tsc_mode = NULL;
int val = 0;
virDomainTimerDefPtr timer;
@@ -509,16 +514,16 @@ xenParseCPUFeatures(virConfPtr conf,
return -1;
}
- if (xenConfigGetString(conf, "cpus", &str, NULL) < 0)
+ if (xenConfigGetString(conf, "cpus", &cpus, NULL) < 0)
return -1;
- if (str && (virBitmapParse(str, &def->cpumask, 4096) < 0))
+ if (cpus && (virBitmapParse(cpus, &def->cpumask, 4096) < 0))
return -1;
- if (xenConfigGetString(conf, "tsc_mode", &str, NULL) < 0)
+ if (xenConfigGetString(conf, "tsc_mode", &tsc_mode, NULL) < 0)
return -1;
- if (str) {
+ if (tsc_mode) {
if (VIR_EXPAND_N(def->clock.timers, def->clock.ntimers, 1) < 0 ||
VIR_ALLOC(timer) < 0)
return -1;
@@ -528,11 +533,11 @@ xenParseCPUFeatures(virConfPtr conf,
timer->tickpolicy = -1;
timer->mode = VIR_DOMAIN_TIMER_MODE_AUTO;
timer->track = -1;
- if (STREQ_NULLABLE(str, "always_emulate"))
+ if (STREQ_NULLABLE(tsc_mode, "always_emulate"))
timer->mode = VIR_DOMAIN_TIMER_MODE_EMULATE;
- else if (STREQ_NULLABLE(str, "native"))
+ else if (STREQ_NULLABLE(tsc_mode, "native"))
timer->mode = VIR_DOMAIN_TIMER_MODE_NATIVE;
- else if (STREQ_NULLABLE(str, "native_paravirt"))
+ else if (STREQ_NULLABLE(tsc_mode, "native_paravirt"))
timer->mode = VIR_DOMAIN_TIMER_MODE_PARAVIRT;
def->clock.timers[def->clock.ntimers - 1] = timer;
@@ -746,15 +751,15 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
static int
xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat)
{
- const char *str;
virConfValuePtr value = NULL;
virDomainChrDefPtr chr = NULL;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
- if (xenConfigGetString(conf, "parallel", &str, NULL) < 0)
+ VIR_AUTOFREE(char *) parallel = NULL;
+ if (xenConfigGetString(conf, "parallel", ¶llel, NULL) < 0)
goto cleanup;
- if (str && STRNEQ(str, "none") &&
- !(chr = xenParseSxprChar(str, NULL)))
+ if (parallel && STRNEQ(parallel, "none") &&
+ !(chr = xenParseSxprChar(parallel, NULL)))
goto cleanup;
if (chr) {
if (VIR_ALLOC_N(def->parallels, 1) < 0)
@@ -801,11 +806,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
value = value->next;
}
} else {
+ VIR_AUTOFREE(char *) serial = NULL;
/* If domain is not using multiple serial ports we parse data old way */
- if (xenConfigGetString(conf, "serial", &str, NULL) < 0)
+ if (xenConfigGetString(conf, "serial", &serial, NULL) < 0)
goto cleanup;
- if (str && STRNEQ(str, "none") &&
- !(chr = xenParseSxprChar(str, NULL)))
+ if (serial && STRNEQ(serial, "none") &&
+ !(chr = xenParseSxprChar(serial, NULL)))
goto cleanup;
if (chr) {
if (VIR_ALLOC_N(def->serials, 1) < 0)
@@ -1049,7 +1055,7 @@ xenParseVifList(virConfPtr conf, virDomainDefPtr def, const char
*vif_typename)
static int
xenParseEmulatedDevices(virConfPtr conf, virDomainDefPtr def)
{
- const char *str;
+ VIR_AUTOFREE(char *) str = NULL;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
if (xenConfigGetString(conf, "soundhw", &str, NULL) < 0)
@@ -1068,7 +1074,7 @@ static int
xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
{
virCapsDomainDataPtr capsdata = NULL;
- const char *str;
+ VIR_AUTOFREE(char *) str = NULL;
int hvm = 0, ret = -1;
if (xenConfigCopyString(conf, "name", &def->name) < 0)
diff --git a/src/xenconfig/xen_common.h b/src/xenconfig/xen_common.h
index 3b7a5db4f3..a26c9e60c4 100644
--- a/src/xenconfig/xen_common.h
+++ b/src/xenconfig/xen_common.h
@@ -33,7 +33,7 @@
int xenConfigGetString(virConfPtr conf,
const char *name,
- const char **value,
+ char **value,
const char *def);
int xenConfigGetBool(virConfPtr conf, const char *name, int *value, int def);
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 19b6604e05..7250e5735d 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -65,7 +65,9 @@ extern int xlu_disk_parse(XLU_Config *cfg,
static int xenParseCmdline(virConfPtr conf, char **r_cmdline)
{
char *cmdline = NULL;
- const char *root, *extra, *buf;
+ VIR_AUTOFREE(char *) root = NULL;
+ VIR_AUTOFREE(char *) extra = NULL;
+ VIR_AUTOFREE(char *) buf = NULL;
if (xenConfigGetString(conf, "cmdline", &buf, NULL) < 0)
return -1;
@@ -104,8 +106,8 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
size_t i;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
- const char *bios;
- const char *boot;
+ VIR_AUTOFREE(char *) bios = NULL;
+ VIR_AUTOFREE(char *) boot = NULL;
int val = 0;
if (xenConfigGetString(conf, "bios", &bios, NULL) < 0)
@@ -255,7 +257,7 @@ xenTranslateCPUFeature(const char *feature_name, bool from_libxl)
static int
xenParseXLCPUID(virConfPtr conf, virDomainDefPtr def)
{
- const char *cpuid_str = NULL;
+ VIR_AUTOFREE(char *) cpuid_str = NULL;
char **cpuid_pairs = NULL;
char **name_and_value = NULL;
size_t i;
diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c
index 7b60f25ec1..909e8fad40 100644
--- a/src/xenconfig/xen_xm.c
+++ b/src/xenconfig/xen_xm.c
@@ -44,7 +44,7 @@ xenParseXMOS(virConfPtr conf, virDomainDefPtr def)
size_t i;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
- const char *boot;
+ VIR_AUTOFREE(char *) boot = NULL;
if (VIR_ALLOC(def->os.loader) < 0 ||
xenConfigCopyString(conf, "kernel",
&def->os.loader->path) < 0)
@@ -72,7 +72,8 @@ xenParseXMOS(virConfPtr conf, virDomainDefPtr def)
def->os.nBootDevs++;
}
} else {
- const char *extra, *root;
+ VIR_AUTOFREE(char *) extra = NULL;
+ VIR_AUTOFREE(char *) root = NULL;
if (xenConfigCopyStringOpt(conf, "bootloader",
&def->os.bootloader) < 0)
return -1;
@@ -417,7 +418,7 @@ xenFormatXMDisks(virConfPtr conf, virDomainDefPtr def)
static int
xenParseXMInputDevs(virConfPtr conf, virDomainDefPtr def)
{
- const char *str;
+ VIR_AUTOFREE(char *) str = NULL;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
if (xenConfigGetString(conf, "usbdevice", &str, NULL) < 0)
--
2.17.1
3:17 p.m.
New subject: [libvirt] [libvirt PATCH v7 3/6] xen_common: Change xenConfigGetString to using virConfGetValueString
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:
This change actually changes the behaviour of xenConfigGetString()
as
now it returns a newly-allocated string.
Unfortunately, there's not much that can be done in order to avoid that
and all the callers have to be changed in order to avoid leaking the
return value.
Also, as a side-effect of the change above, the function now takes a
"char **" argument instead of a "const char **" one.
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
---
src/xenconfig/xen_common.c | 84 ++++++++++++++++++++------------------
src/xenconfig/xen_common.h | 2 +-
src/xenconfig/xen_xl.c | 10 +++--
src/xenconfig/xen_xm.c | 7 ++--
4 files changed, 56 insertions(+), 47 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 587bab2b19..7b3e5c3b44 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -228,26 +228,28 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char
*uuid)
int
xenConfigGetString(virConfPtr conf,
const char *name,
- const char **value,
+ char **value,
const char *def)
{
- virConfValuePtr val;
+ char *string = NULL;
+ int rc;
*value = NULL;
- if (!(val = virConfGetValue(conf, name))) {
- *value = def;
+ if ((rc = virConfGetValueString(conf, name, &string)) < 0)
+ return -1;
+
+ if (rc == 0) {
+ if (VIR_STRDUP(*value, def) < 0)
+ return -1;
return 0;
}
- if (val->type != VIR_CONF_STRING) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("config value %s was malformed"), name);
- return -1;
+ if (!string) {
+ if (VIR_STRDUP(*value, def) < 0)
+ return -1;
+ } else {
+ *value = string;
}
- if (!val->str)
- *value = def;
- else
- *value = val->str;
I think this all can be further simplified to:
if (rc == 0 || !string) {
if (VIR_STRDUP(*value, def) < 0)
return -1;
} else {
*value = string;
}
return 0;
return 0;
}
@@ -345,32 +347,34 @@ xenParseTimeOffset(virConfPtr conf, virDomainDefPtr def)
static int
xenParseEventsActions(virConfPtr conf, virDomainDefPtr def)
{
- const char *str = NULL;
+ VIR_AUTOFREE(char *) on_poweroff = NULL;
+ VIR_AUTOFREE(char *) on_reboot = NULL;
+ VIR_AUTOFREE(char *) on_crash = NULL;
- if (xenConfigGetString(conf, "on_poweroff", &str, "destroy")
< 0)
+ if (xenConfigGetString(conf, "on_poweroff", &on_poweroff,
"destroy") < 0)
return -1;
- if ((def->onPoweroff = virDomainLifecycleActionTypeFromString(str)) < 0) {
+ if ((def->onPoweroff = virDomainLifecycleActionTypeFromString(on_poweroff)) <
0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected value %s for on_poweroff"), str);
+ _("unexpected value %s for on_poweroff"),
on_poweroff);
return -1;
}
- if (xenConfigGetString(conf, "on_reboot", &str, "restart")
< 0)
+ if (xenConfigGetString(conf, "on_reboot", &on_reboot,
"restart") < 0)
return -1;
- if ((def->onReboot = virDomainLifecycleActionTypeFromString(str)) < 0) {
+ if ((def->onReboot = virDomainLifecycleActionTypeFromString(on_reboot)) < 0)
{
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected value %s for on_reboot"), str);
+ _("unexpected value %s for on_reboot"), on_reboot);
return -1;
}
- if (xenConfigGetString(conf, "on_crash", &str, "restart")
< 0)
+ if (xenConfigGetString(conf, "on_crash", &on_crash,
"restart") < 0)
return -1;
- if ((def->onCrash = virDomainLifecycleActionTypeFromString(str)) < 0) {
+ if ((def->onCrash = virDomainLifecycleActionTypeFromString(on_crash)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected value %s for on_crash"), str);
+ _("unexpected value %s for on_crash"), on_crash);
return -1;
}
@@ -488,7 +492,8 @@ xenParseCPUFeatures(virConfPtr conf,
virDomainXMLOptionPtr xmlopt)
{
unsigned long count = 0;
- const char *str = NULL;
+ VIR_AUTOFREE(char *) cpus = NULL;
+ VIR_AUTOFREE(char *) tsc_mode = NULL;
int val = 0;
virDomainTimerDefPtr timer;
@@ -509,16 +514,16 @@ xenParseCPUFeatures(virConfPtr conf,
return -1;
}
- if (xenConfigGetString(conf, "cpus", &str, NULL) < 0)
+ if (xenConfigGetString(conf, "cpus", &cpus, NULL) < 0)
return -1;
- if (str && (virBitmapParse(str, &def->cpumask, 4096) < 0))
+ if (cpus && (virBitmapParse(cpus, &def->cpumask, 4096) < 0))
return -1;
- if (xenConfigGetString(conf, "tsc_mode", &str, NULL) < 0)
+ if (xenConfigGetString(conf, "tsc_mode", &tsc_mode, NULL) < 0)
return -1;
- if (str) {
+ if (tsc_mode) {
if (VIR_EXPAND_N(def->clock.timers, def->clock.ntimers, 1) < 0 ||
VIR_ALLOC(timer) < 0)
return -1;
@@ -528,11 +533,11 @@ xenParseCPUFeatures(virConfPtr conf,
timer->tickpolicy = -1;
timer->mode = VIR_DOMAIN_TIMER_MODE_AUTO;
timer->track = -1;
- if (STREQ_NULLABLE(str, "always_emulate"))
+ if (STREQ_NULLABLE(tsc_mode, "always_emulate"))
timer->mode = VIR_DOMAIN_TIMER_MODE_EMULATE;
- else if (STREQ_NULLABLE(str, "native"))
+ else if (STREQ_NULLABLE(tsc_mode, "native"))
timer->mode = VIR_DOMAIN_TIMER_MODE_NATIVE;
- else if (STREQ_NULLABLE(str, "native_paravirt"))
+ else if (STREQ_NULLABLE(tsc_mode, "native_paravirt"))
Don't believe the _NULLABLE variant is/was required here considering
@str couldn't have been NULL and certainly the right side isn't either!
Jim must've been super-paranoid for commit b4386fdac or perhaps worried
about the default value of NULL.
timer->mode = VIR_DOMAIN_TIMER_MODE_PARAVIRT;
def->clock.timers[def->clock.ntimers - 1] = timer;
@@ -746,15 +751,15 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
static int
xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat)
{
- const char *str;
virConfValuePtr value = NULL;
virDomainChrDefPtr chr = NULL;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
- if (xenConfigGetString(conf, "parallel", &str, NULL) < 0)
+ VIR_AUTOFREE(char *) parallel = NULL;
+ if (xenConfigGetString(conf, "parallel", ¶llel, NULL) < 0)
goto cleanup;
- if (str && STRNEQ(str, "none") &&
- !(chr = xenParseSxprChar(str, NULL)))
+ if (parallel && STRNEQ(parallel, "none") &&
+ !(chr = xenParseSxprChar(parallel, NULL)))
Then there's this one where STRNEQ_NULLABLE would be the same check,
just like the next one for @serial...
Again, I won't change - unless you think we should just change these...
goto cleanup;
if (chr) {
if (VIR_ALLOC_N(def->parallels, 1) < 0)
@@ -801,11 +806,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
value = value->next;
}
} else {
+ VIR_AUTOFREE(char *) serial = NULL;
/* If domain is not using multiple serial ports we parse data old way */
- if (xenConfigGetString(conf, "serial", &str, NULL) < 0)
+ if (xenConfigGetString(conf, "serial", &serial, NULL) < 0)
goto cleanup;
- if (str && STRNEQ(str, "none") &&
- !(chr = xenParseSxprChar(str, NULL)))
+ if (serial && STRNEQ(serial, "none") &&
+ !(chr = xenParseSxprChar(serial, NULL)))
goto cleanup;
if (chr) {
if (VIR_ALLOC_N(def->serials, 1) < 0)
[...]
I can make the changes locally before pushing - no need for another
series... I'll do the simplification of the logic, but hold off on the
_NULLABLE changes unless you think those are worth changing too.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
3:35 p.m.
New subject: [libvirt] [libvirt PATCH v7 3/6] xen_common: Change xenConfigGetString to using virConfGetValueString
On Thu, Sep 20, 2018 at 10:17 PM, John Ferlan <jferlan(a)redhat.com> wrote:
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:
> This change actually changes the behaviour of xenConfigGetString() as
> now it returns a newly-allocated string.
>
> Unfortunately, there's not much that can be done in order to avoid that
> and all the callers have to be changed in order to avoid leaking the
> return value.
>
> Also, as a side-effect of the change above, the function now takes a
> "char **" argument instead of a "const char **" one.
>
> Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
> ---
> src/xenconfig/xen_common.c | 84 ++++++++++++++++++++------------------
> src/xenconfig/xen_common.h | 2 +-
> src/xenconfig/xen_xl.c | 10 +++--
> src/xenconfig/xen_xm.c | 7 ++--
> 4 files changed, 56 insertions(+), 47 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 587bab2b19..7b3e5c3b44 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -228,26 +228,28 @@ xenConfigGetUUID(virConfPtr conf, const char
*name, unsigned char *uuid)
> int
> xenConfigGetString(virConfPtr conf,
> const char *name,
> - const char **value,
> + char **value,
> const char *def)
> {
> - virConfValuePtr val;
> + char *string = NULL;
> + int rc;
>
> *value = NULL;
> - if (!(val = virConfGetValue(conf, name))) {
> - *value = def;
> + if ((rc = virConfGetValueString(conf, name, &string)) < 0)
> + return -1;
> +
> + if (rc == 0) {
> + if (VIR_STRDUP(*value, def) < 0)
> + return -1;
> return 0;
> }
>
> - if (val->type != VIR_CONF_STRING) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("config value %s was malformed"), name);
> - return -1;
> + if (!string) {
> + if (VIR_STRDUP(*value, def) < 0)
> + return -1;
> + } else {
> + *value = string;
> }
> - if (!val->str)
> - *value = def;
> - else
> - *value = val->str;
I think this all can be further simplified to:
if (rc == 0 || !string) {
if (VIR_STRDUP(*value, def) < 0)
return -1;
} else {
*value = string;
}
return 0;
The only reason I didn't go for it was to have the diff cleaner/smaller.
If you're fine merging the "ifs", please, go for it.
> return 0;
> }
>
> @@ -345,32 +347,34 @@ xenParseTimeOffset(virConfPtr conf,
virDomainDefPtr def)
> static int
> xenParseEventsActions(virConfPtr conf, virDomainDefPtr def)
> {
> - const char *str = NULL;
> + VIR_AUTOFREE(char *) on_poweroff = NULL;
> + VIR_AUTOFREE(char *) on_reboot = NULL;
> + VIR_AUTOFREE(char *) on_crash = NULL;
>
> - if (xenConfigGetString(conf, "on_poweroff", &str,
"destroy") < 0)
> + if (xenConfigGetString(conf, "on_poweroff", &on_poweroff,
"destroy") < 0)
> return -1;
>
> - if ((def->onPoweroff = virDomainLifecycleActionTypeFromString(str))
< 0) {
> + if ((def->onPoweroff = virDomainLifecycleActionTypeFromString(on_poweroff))
< 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unexpected value %s for on_poweroff"), str);
> + _("unexpected value %s for on_poweroff"),
on_poweroff);
> return -1;
> }
>
> - if (xenConfigGetString(conf, "on_reboot", &str,
"restart") < 0)
> + if (xenConfigGetString(conf, "on_reboot", &on_reboot,
"restart") <
0)
> return -1;
>
> - if ((def->onReboot = virDomainLifecycleActionTypeFromString(str))
< 0) {
> + if ((def->onReboot = virDomainLifecycleActionTypeFromString(on_reboot))
< 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unexpected value %s for on_reboot"), str);
> + _("unexpected value %s for on_reboot"),
on_reboot);
> return -1;
> }
>
> - if (xenConfigGetString(conf, "on_crash", &str,
"restart") < 0)
> + if (xenConfigGetString(conf, "on_crash", &on_crash,
"restart") < 0)
> return -1;
>
> - if ((def->onCrash = virDomainLifecycleActionTypeFromString(str)) <
0) {
> + if ((def->onCrash = virDomainLifecycleActionTypeFromString(on_crash))
< 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unexpected value %s for on_crash"), str);
> + _("unexpected value %s for on_crash"), on_crash);
> return -1;
> }
>
> @@ -488,7 +492,8 @@ xenParseCPUFeatures(virConfPtr conf,
> virDomainXMLOptionPtr xmlopt)
> {
> unsigned long count = 0;
> - const char *str = NULL;
> + VIR_AUTOFREE(char *) cpus = NULL;
> + VIR_AUTOFREE(char *) tsc_mode = NULL;
> int val = 0;
> virDomainTimerDefPtr timer;
>
> @@ -509,16 +514,16 @@ xenParseCPUFeatures(virConfPtr conf,
> return -1;
> }
>
> - if (xenConfigGetString(conf, "cpus", &str, NULL) < 0)
> + if (xenConfigGetString(conf, "cpus", &cpus, NULL) < 0)
> return -1;
>
> - if (str && (virBitmapParse(str, &def->cpumask, 4096) < 0))
> + if (cpus && (virBitmapParse(cpus, &def->cpumask, 4096) < 0))
> return -1;
>
> - if (xenConfigGetString(conf, "tsc_mode", &str, NULL) < 0)
> + if (xenConfigGetString(conf, "tsc_mode", &tsc_mode, NULL) <
0)
> return -1;
>
> - if (str) {
> + if (tsc_mode) {
> if (VIR_EXPAND_N(def->clock.timers, def->clock.ntimers, 1) < 0
||
> VIR_ALLOC(timer) < 0)
> return -1;
> @@ -528,11 +533,11 @@ xenParseCPUFeatures(virConfPtr conf,
> timer->tickpolicy = -1;
> timer->mode = VIR_DOMAIN_TIMER_MODE_AUTO;
> timer->track = -1;
> - if (STREQ_NULLABLE(str, "always_emulate"))
> + if (STREQ_NULLABLE(tsc_mode, "always_emulate"))
> timer->mode = VIR_DOMAIN_TIMER_MODE_EMULATE;
> - else if (STREQ_NULLABLE(str, "native"))
> + else if (STREQ_NULLABLE(tsc_mode, "native"))
> timer->mode = VIR_DOMAIN_TIMER_MODE_NATIVE;
> - else if (STREQ_NULLABLE(str, "native_paravirt"))
> + else if (STREQ_NULLABLE(tsc_mode, "native_paravirt"))
Don't believe the _NULLABLE variant is/was required here considering
@str couldn't have been NULL and certainly the right side isn't either!
Jim must've been super-paranoid for commit b4386fdac or perhaps worried
about the default value of NULL.
> timer->mode = VIR_DOMAIN_TIMER_MODE_PARAVIRT;
>
> def->clock.timers[def->clock.ntimers - 1] = timer;
> @@ -746,15 +751,15 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> static int
> xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
> {
> - const char *str;
> virConfValuePtr value = NULL;
> virDomainChrDefPtr chr = NULL;
>
> if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> - if (xenConfigGetString(conf, "parallel", &str, NULL) < 0)
> + VIR_AUTOFREE(char *) parallel = NULL;
> + if (xenConfigGetString(conf, "parallel", ¶llel, NULL)
< 0)
> goto cleanup;
> - if (str && STRNEQ(str, "none") &&
> - !(chr = xenParseSxprChar(str, NULL)))
> + if (parallel && STRNEQ(parallel, "none") &&
> + !(chr = xenParseSxprChar(parallel, NULL)))
Then there's this one where STRNEQ_NULLABLE would be the same check,
just like the next one for @serial...
Again, I won't change - unless you think we should just change these...
> goto cleanup;
> if (chr) {
> if (VIR_ALLOC_N(def->parallels, 1) < 0)
> @@ -801,11 +806,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
def, const char *nativeFormat)
> value = value->next;
> }
> } else {
> + VIR_AUTOFREE(char *) serial = NULL;
> /* If domain is not using multiple serial ports we parse
data old way */
> - if (xenConfigGetString(conf, "serial", &str, NULL) <
0)
> + if (xenConfigGetString(conf, "serial", &serial, NULL)
< 0)
> goto cleanup;
> - if (str && STRNEQ(str, "none") &&
> - !(chr = xenParseSxprChar(str, NULL)))
> + if (serial && STRNEQ(serial, "none") &&
> + !(chr = xenParseSxprChar(serial, NULL)))
> goto cleanup;
> if (chr) {
> if (VIR_ALLOC_N(def->serials, 1) < 0)
[...]
I can make the changes locally before pushing - no need for another
series... I'll do the simplification of the logic, but hold off on the
_NULLABLE changes unless you think those are worth changing too.
I totally agree and I don't think the _NULLABLE changes should be part of
this commit/series.
Please, do the changes! :-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
8:28 a.m.
New subject: [libvirt] [libvirt PATCH v7 4/6] xen_common: Change xenParsePCIList to using virConfGetValueStringList
The `if(!list || list->type != VIR_CONF_LIST)` check couldn't be
written in a 100% similar way. Instead, we're just checking whether
`virConfGetValueStringList() <= 0` and creating a new function to:
- return -1 in case virConfGetValueStringList fails either due to some
allocation failure or when traversing the list;
- resetting the last error and return 0 otherwise;
Taking this approach we can have the behaviour with the new code as
close as possible to the old one.
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
---
src/xenconfig/xen_common.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 7b3e5c3b44..9133998cd7 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -458,22 +458,40 @@ xenParsePCI(char *entry)
return hostdev;
}
+static int
+xenHandleConfGetValueStringListErrors(int ret, int errorCode)
+{
+ if (ret < 0) {
+ /* It means virConfGetValueStringList() didn't fail because the
+ * cval->type switch fell through - since we're passing
+ * @compatString == false - assumes failures for memory allocation
+ * and VIR_CONF_LIST traversal failure should cause -1 to be
+ * returned to the caller with the error message set. */
+ if (errorCode != VIR_ERR_INTERNAL_ERROR)
+ return -1;
+
+ /* If we did fall through the switch, then ignore and clear the
+ * last error. */
+ virResetLastError();
+ }
+ return 0;
+}
static int
xenParsePCIList(virConfPtr conf, virDomainDefPtr def)
{
- virConfValuePtr list = virConfGetValue(conf, "pci");
+ VIR_AUTOPTR(virString) pcis = NULL;
+ virString *entries = NULL;
+ int rc;
- if (!list || list->type != VIR_CONF_LIST)
- return 0;
+ if ((rc = virConfGetValueStringList(conf, "pci", false, &pcis)) <=
0)
+ return xenHandleConfGetValueStringListErrors(rc, virGetLastErrorCode());
- for (list = list->list; list; list = list->next) {
+ for (entries = pcis; *entries; entries++) {
+ virString entry = *entries;
virDomainHostdevDefPtr hostdev;
- if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
- continue;
-
- if (!(hostdev = xenParsePCI(list->str)))
+ if (!(hostdev = xenParsePCI(entry)))
return -1;
if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) {
--
2.17.1
3:18 p.m.
New subject: [libvirt] [libvirt PATCH v7 4/6] xen_common: Change xenParsePCIList to using virConfGetValueStringList
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:
The `if(!list || list->type != VIR_CONF_LIST)` check couldn't
be
written in a 100% similar way. Instead, we're just checking whether
`virConfGetValueStringList() <= 0` and creating a new function to:
- return -1 in case virConfGetValueStringList fails either due to some
allocation failure or when traversing the list;
- resetting the last error and return 0 otherwise;
Taking this approach we can have the behaviour with the new code as
close as possible to the old one.
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
---
src/xenconfig/xen_common.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 7b3e5c3b44..9133998cd7 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -458,22 +458,40 @@ xenParsePCI(char *entry)
return hostdev;
}
Things you will get used to eventually... New functions with 2 blank
lines and each argument on one line, although I don't believe you need
to pass errorCode here.
+static int
+xenHandleConfGetValueStringListErrors(int ret, int errorCode)
+{
+ if (ret < 0) {
+ /* It means virConfGetValueStringList() didn't fail because the
+ * cval->type switch fell through - since we're passing
+ * @compatString == false - assumes failures for memory allocation
+ * and VIR_CONF_LIST traversal failure should cause -1 to be
+ * returned to the caller with the error message set. */
+ if (errorCode != VIR_ERR_INTERNAL_ERROR)
+ return -1;
+
+ /* If we did fall through the switch, then ignore and clear the
+ * last error. */
+ virResetLastError();
+ }
+ return 0;
+}
static int
xenParsePCIList(virConfPtr conf, virDomainDefPtr def)
{
- virConfValuePtr list = virConfGetValue(conf, "pci");
+ VIR_AUTOPTR(virString) pcis = NULL;
+ virString *entries = NULL;
+ int rc;
- if (!list || list->type != VIR_CONF_LIST)
- return 0;
+ if ((rc = virConfGetValueStringList(conf, "pci", false, &pcis)) <=
0)
+ return xenHandleConfGetValueStringListErrors(rc, virGetLastErrorCode());
No need to pass virGetLastErrorCode - in the 3 uses I see it's not
changing and the called method can make that call.
I can alter before pushing if you're fine with that.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
[...]
3:36 p.m.
New subject: [libvirt] [libvirt PATCH v7 4/6] xen_common: Change xenParsePCIList to using virConfGetValueStringList
On Thu, Sep 20, 2018 at 10:18 PM, John Ferlan <jferlan(a)redhat.com> wrote:
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:
> The `if(!list || list->type != VIR_CONF_LIST)` check couldn't be
> written in a 100% similar way. Instead, we're just checking whether
> `virConfGetValueStringList() <= 0` and creating a new function to:
> - return -1 in case virConfGetValueStringList fails either due to some
> allocation failure or when traversing the list;
> - resetting the last error and return 0 otherwise;
>
> Taking this approach we can have the behaviour with the new code as
> close as possible to the old one.
>
> Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
> ---
> src/xenconfig/xen_common.c | 34 ++++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 7b3e5c3b44..9133998cd7 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -458,22 +458,40 @@ xenParsePCI(char *entry)
> return hostdev;
> }
>
Things you will get used to eventually... New functions with 2 blank
lines and each argument on one line, although I don't believe you need
to pass errorCode here.
Okay, noted!
> +static int
> +xenHandleConfGetValueStringListErrors(int ret, int errorCode)
> +{
> + if (ret < 0) {
> + /* It means virConfGetValueStringList() didn't fail because the
> + * cval->type switch fell through - since we're passing
> + * @compatString == false - assumes failures for memory
allocation
> + * and VIR_CONF_LIST traversal failure should cause -1 to be
> + * returned to the caller with the error message set. */
> + if (errorCode != VIR_ERR_INTERNAL_ERROR)
> + return -1;
> +
> + /* If we did fall through the switch, then ignore and clear the
> + * last error. */
> + virResetLastError();
> + }
> + return 0;
> +}
>
> static int
> xenParsePCIList(virConfPtr conf, virDomainDefPtr def)
> {
> - virConfValuePtr list = virConfGetValue(conf, "pci");
> + VIR_AUTOPTR(virString) pcis = NULL;
> + virString *entries = NULL;
> + int rc;
>
> - if (!list || list->type != VIR_CONF_LIST)
> - return 0;
> + if ((rc = virConfGetValueStringList(conf, "pci", false, &pcis))
<=
0)
> + return xenHandleConfGetValueStringListErrors(rc,
virGetLastErrorCode());
No need to pass virGetLastErrorCode - in the 3 uses I see it's not
changing and the called method can make that call.
I can alter before pushing if you're fine with that.
Yes, I'm fine with that. Please, do the changes before pushing! :-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
[...]
8:28 a.m.
New subject: [libvirt] [libvirt PATCH v7 5/6] xen_common: Change xenParseVfbs to using virConfGetValueStringList
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
---
src/xenconfig/xen_common.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 9133998cd7..058f35825e 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -618,7 +618,6 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
int val;
char *listenAddr = NULL;
int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
- virConfValuePtr list;
virDomainGraphicsDefPtr graphics = NULL;
if (hvm) {
@@ -674,17 +673,17 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
}
if (!hvm && def->graphics == NULL) { /* New PV guests use this format */
- list = virConfGetValue(conf, "vfb");
- if (list && list->type == VIR_CONF_LIST &&
- list->list && list->list->type == VIR_CONF_STRING
&&
- list->list->str) {
+ VIR_AUTOPTR(virString) vfbs = NULL;
+ int rc;
+
+ if ((rc = virConfGetValueStringList(conf, "vfb", false, &vfbs)) ==
1) {
char vfb[MAX_VFB];
char *key = vfb;
- if (virStrcpyStatic(vfb, list->list->str) < 0) {
+ if (virStrcpyStatic(vfb, *vfbs) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("VFB %s too big for destination"),
- list->list->str);
+ *vfbs);
goto cleanup;
}
@@ -754,6 +753,11 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
def->graphics[0] = graphics;
def->ngraphics = 1;
graphics = NULL;
+ } else {
+ rc = xenHandleConfGetValueStringListErrors(rc,
+ virGetLastErrorCode());
+ if (rc < 0)
+ goto cleanup;
}
}
--
2.17.1
3:19 p.m.
New subject: [libvirt] [libvirt PATCH v7 5/6] xen_common: Change xenParseVfbs to using virConfGetValueStringList
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
---
src/xenconfig/xen_common.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 9133998cd7..058f35825e 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -618,7 +618,6 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
int val;
char *listenAddr = NULL;
int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
- virConfValuePtr list;
virDomainGraphicsDefPtr graphics = NULL;
if (hvm) {
@@ -674,17 +673,17 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
}
if (!hvm && def->graphics == NULL) { /* New PV guests use this format */
- list = virConfGetValue(conf, "vfb");
- if (list && list->type == VIR_CONF_LIST &&
- list->list && list->list->type == VIR_CONF_STRING
&&
- list->list->str) {
+ VIR_AUTOPTR(virString) vfbs = NULL;
+ int rc;
+
+ if ((rc = virConfGetValueStringList(conf, "vfb", false, &vfbs)) ==
1) {
char vfb[MAX_VFB];
char *key = vfb;
- if (virStrcpyStatic(vfb, list->list->str) < 0) {
+ if (virStrcpyStatic(vfb, *vfbs) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("VFB %s too big for destination"),
- list->list->str);
+ *vfbs);
goto cleanup;
}
@@ -754,6 +753,11 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
def->graphics[0] = graphics;
def->ngraphics = 1;
graphics = NULL;
+ } else {
+ rc = xenHandleConfGetValueStringListErrors(rc,
+ virGetLastErrorCode());
+ if (rc < 0)
Based on patch4 change, this is simplified to just:
if (xenHandleConfGetValueStringListErrors(rc) < 0)
I can alter before pushing if you're fine with that.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
+ goto cleanup;
}
}
3:36 p.m.
New subject: [libvirt] [libvirt PATCH v7 5/6] xen_common: Change xenParseVfbs to using virConfGetValueStringList
On Thu, Sep 20, 2018 at 10:19 PM, John Ferlan <jferlan(a)redhat.com> wrote:
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:
> Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
> ---
> src/xenconfig/xen_common.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 9133998cd7..058f35825e 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -618,7 +618,6 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> int val;
> char *listenAddr = NULL;
> int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
> - virConfValuePtr list;
> virDomainGraphicsDefPtr graphics = NULL;
>
> if (hvm) {
> @@ -674,17 +673,17 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> }
>
> if (!hvm && def->graphics == NULL) { /* New PV guests use this
format */
> - list = virConfGetValue(conf, "vfb");
> - if (list && list->type == VIR_CONF_LIST &&
> - list->list && list->list->type == VIR_CONF_STRING
&&
> - list->list->str) {
> + VIR_AUTOPTR(virString) vfbs = NULL;
> + int rc;
> +
> + if ((rc = virConfGetValueStringList(conf, "vfb", false,
&vfbs)) == 1) {
> char vfb[MAX_VFB];
> char *key = vfb;
>
> - if (virStrcpyStatic(vfb, list->list->str) < 0) {
> + if (virStrcpyStatic(vfb, *vfbs) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("VFB %s too big for destination"),
> - list->list->str);
> + *vfbs);
> goto cleanup;
> }
>
> @@ -754,6 +753,11 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> def->graphics[0] = graphics;
> def->ngraphics = 1;
> graphics = NULL;
> + } else {
> + rc = xenHandleConfGetValueStringListErrors(rc,
> +
virGetLastErrorCode());
> + if (rc < 0)
Based on patch4 change, this is simplified to just:
if (xenHandleConfGetValueStringListErrors(rc) < 0)
I can alter before pushing if you're fine with that.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Sure!
John
> + goto cleanup;
> }
> }
>
>
8:28 a.m.
New subject: [libvirt] [libvirt PATCH v7 6/6] xen_common: Change xenParseCharDev to using virConfGetValueStringList
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
---
src/xenconfig/xen_common.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 058f35825e..21f1f4a24c 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -773,11 +773,13 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
static int
xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat)
{
- virConfValuePtr value = NULL;
+ VIR_AUTOPTR(virString) serials = NULL;
virDomainChrDefPtr chr = NULL;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
VIR_AUTOFREE(char *) parallel = NULL;
+ int rc;
+
if (xenConfigGetString(conf, "parallel", ¶llel, NULL) < 0)
goto cleanup;
if (parallel && STRNEQ(parallel, "none") &&
@@ -795,8 +797,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
}
/* Try to get the list of values to support multiple serial ports */
- value = virConfGetValue(conf, "serial");
- if (value && value->type == VIR_CONF_LIST) {
+ if ((rc = virConfGetValueStringList(conf, "serial", false,
&serials)) == 1) {
+ virString *entries;
int portnum = -1;
if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) {
@@ -805,18 +807,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
goto cleanup;
}
- value = value->list;
- while (value) {
- char *port = NULL;
+ for (entries = serials; *entries; entries++) {
+ virString port = *entries;
- if ((value->type != VIR_CONF_STRING) || (value->str == NULL))
- goto cleanup;
- port = value->str;
portnum++;
- if (STREQ(port, "none")) {
- value = value->next;
+ if (STREQ(port, "none"))
continue;
- }
if (!(chr = xenParseSxprChar(port, NULL)))
goto cleanup;
@@ -824,11 +820,14 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
chr->target.port = portnum;
if (VIR_APPEND_ELEMENT(def->serials, def->nserials, chr) < 0)
goto cleanup;
-
- value = value->next;
}
} else {
VIR_AUTOFREE(char *) serial = NULL;
+
+ rc = xenHandleConfGetValueStringListErrors(rc, virGetLastErrorCode());
+ if (rc < 0)
+ goto cleanup;
+
/* If domain is not using multiple serial ports we parse data old way */
if (xenConfigGetString(conf, "serial", &serial, NULL) < 0)
goto cleanup;
--
2.17.1
3:20 p.m.
New subject: [libvirt] [libvirt PATCH v7 6/6] xen_common: Change xenParseCharDev to using virConfGetValueStringList
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
---
src/xenconfig/xen_common.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 058f35825e..21f1f4a24c 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -773,11 +773,13 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
static int
xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat)
{
- virConfValuePtr value = NULL;
+ VIR_AUTOPTR(virString) serials = NULL;
virDomainChrDefPtr chr = NULL;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
VIR_AUTOFREE(char *) parallel = NULL;
+ int rc;
+
if (xenConfigGetString(conf, "parallel", ¶llel, NULL) < 0)
goto cleanup;
if (parallel && STRNEQ(parallel, "none") &&
@@ -795,8 +797,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
}
/* Try to get the list of values to support multiple serial ports */
- value = virConfGetValue(conf, "serial");
- if (value && value->type == VIR_CONF_LIST) {
+ if ((rc = virConfGetValueStringList(conf, "serial", false,
&serials)) == 1) {
+ virString *entries;
int portnum = -1;
if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) {
@@ -805,18 +807,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
goto cleanup;
}
- value = value->list;
- while (value) {
- char *port = NULL;
+ for (entries = serials; *entries; entries++) {
+ virString port = *entries;
- if ((value->type != VIR_CONF_STRING) || (value->str == NULL))
- goto cleanup;
- port = value->str;
portnum++;
- if (STREQ(port, "none")) {
- value = value->next;
+ if (STREQ(port, "none"))
continue;
- }
if (!(chr = xenParseSxprChar(port, NULL)))
goto cleanup;
@@ -824,11 +820,14 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
chr->target.port = portnum;
if (VIR_APPEND_ELEMENT(def->serials, def->nserials, chr) < 0)
goto cleanup;
-
- value = value->next;
}
} else {
VIR_AUTOFREE(char *) serial = NULL;
+
+ rc = xenHandleConfGetValueStringListErrors(rc, virGetLastErrorCode());
+ if (rc < 0)
Based on patch4 change, this is simplified to just:
if (xenHandleConfGetValueStringListErrors(rc) < 0)
I can alter before pushing if you're fine with that.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
Logic is sooo much nicer with these VIR_AUTOFREE and VIR_AUTOPTR!
+ goto cleanup;
+
/* If domain is not using multiple serial ports we parse data old way */
if (xenConfigGetString(conf, "serial", &serial, NULL) < 0)
goto cleanup;
3:37 p.m.
New subject: [libvirt] [libvirt PATCH v7 6/6] xen_common: Change xenParseCharDev to using virConfGetValueStringList
On Thu, Sep 20, 2018 at 10:20 PM, John Ferlan <jferlan(a)redhat.com> wrote:
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:
> Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
> ---
> src/xenconfig/xen_common.c | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 058f35825e..21f1f4a24c 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -773,11 +773,13 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> static int
> xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
> {
> - virConfValuePtr value = NULL;
> + VIR_AUTOPTR(virString) serials = NULL;
> virDomainChrDefPtr chr = NULL;
>
> if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> VIR_AUTOFREE(char *) parallel = NULL;
> + int rc;
> +
> if (xenConfigGetString(conf, "parallel", ¶llel, NULL)
< 0)
> goto cleanup;
> if (parallel && STRNEQ(parallel, "none") &&
> @@ -795,8 +797,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
def, const char *nativeFormat)
> }
>
> /* Try to get the list of values to support multiple serial
ports */
> - value = virConfGetValue(conf, "serial");
> - if (value && value->type == VIR_CONF_LIST) {
> + if ((rc = virConfGetValueStringList(conf, "serial", false,
&serials)) == 1) {
> + virString *entries;
> int portnum = -1;
>
> if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) {
> @@ -805,18 +807,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
def, const char *nativeFormat)
> goto cleanup;
> }
>
> - value = value->list;
> - while (value) {
> - char *port = NULL;
> + for (entries = serials; *entries; entries++) {
> + virString port = *entries;
>
> - if ((value->type != VIR_CONF_STRING) || (value->str ==
NULL))
> - goto cleanup;
> - port = value->str;
> portnum++;
> - if (STREQ(port, "none")) {
> - value = value->next;
> + if (STREQ(port, "none"))
> continue;
> - }
>
> if (!(chr = xenParseSxprChar(port, NULL)))
> goto cleanup;
> @@ -824,11 +820,14 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
def, const char *nativeFormat)
> chr->target.port = portnum;
> if (VIR_APPEND_ELEMENT(def->serials, def->nserials,
chr) < 0)
> goto cleanup;
> -
> - value = value->next;
> }
> } else {
> VIR_AUTOFREE(char *) serial = NULL;
> +
> + rc = xenHandleConfGetValueStringListErrors(rc,
virGetLastErrorCode());
> + if (rc < 0)
Based on patch4 change, this is simplified to just:
if (xenHandleConfGetValueStringListErrors(rc) < 0)
I can alter before pushing if you're fine with that.
Sure. Thanks a lot for the review, John!
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
Logic is sooo much nicer with these VIR_AUTOFREE and VIR_AUTOPTR!
> + goto cleanup;
> +
> /* If domain is not using multiple serial ports we parse
data old way */
> if (xenConfigGetString(conf, "serial", &serial, NULL)
< 0)
> goto cleanup;
>
2291
days inactive
2291
days old
14 comments
2 participants
participants (2)
-
Fabiano Fidêncio -
John Ferlan