[libvirt] [libvirt PATCH v6 00/15] Finish the conversion to virConfGetValue* functions libvirt
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_virConfGet... 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. Fabiano Fidêncio (15): xen_common: Change xenConfigCopyStringInternal to using virConfGetValueString xen_common: Change xenConfigGetUUID to using virConfGetValueString xen_common: Change xenConfigGetString to using virConfGetValueString xen_xl: Adapt xenParseCmdline due to changes in xenConfigGetString xen_xl: Adapt xenParseXLOS due to changes in xenConfigGetString xen_xl: adapt xenParseXLCPUID due to changes in xenConfigGetString xen_common: Adapt xenParseEventsActions due to changes in xenConfigGetString xen_common: Adapt xenParseCPUFeatures due to changes in xenConfigGetString xen_common: Adapt xenParseEmulatedDevices due to changes in xenConfigGetString xen_common: Adapt xenParseGeneralMeta due to changes in xenConfigGetString xen_xm: Adapt xenParseXMOS due to changes in xenConfigGetString xen_xm: Adapt xenParseXMInputDevs due to changes in xenConfigGetString 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 | 223 +++++++++++++++++++------------------ src/xenconfig/xen_xl.c | 71 +++++++----- src/xenconfig/xen_xm.c | 48 ++++---- 3 files changed, 186 insertions(+), 156 deletions(-) -- 2.17.1
Signed-off-by: Fabiano Fidêncio <fidencio@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
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index a35e1aff58..08fbfff44f 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -183,7 +183,7 @@ xenConfigCopyStringOpt(virConfPtr conf, const char *name, char **value) static int xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) { - virConfValuePtr val; + char *string = NULL; if (!uuid || !name || !conf) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -191,7 +191,7 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) return -1; } - if (!(val = virConfGetValue(conf, name))) { + if (virConfGetValueString(conf, name, &string) <= 0) { if (virUUIDGenerate(uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); @@ -201,24 +201,20 @@ 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); + VIR_FREE(string); return -1; } + VIR_FREE(string); return 0; } -- 2.17.1
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index a35e1aff58..08fbfff44f 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -183,7 +183,7 @@ xenConfigCopyStringOpt(virConfPtr conf, const char *name, char **value) static int xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) { - virConfValuePtr val; + char *string = NULL;
if (!uuid || !name || !conf) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -191,7 +191,7 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) return -1; }
- if (!(val = virConfGetValue(conf, name))) {
This is the "isn't found" or "was missing" test...
+ if (virConfGetValueString(conf, name, &string) <= 0) {
But this takes it one step too far. If return < 0, then we had "if (cval->type != VIR_CONF_STRING)" or "VIR_STRDUP" failure. However, if you change to: int rc; ... if ((rc = virConfGetValueString(conf, name, &string)) < 0) return -1; if (rc == 0) { then I believe we're good. With the change, Reviewed-by: John Ferlan <jferlan@redhat.com> John [I can make the change locally, but I've forgotten the magic incantation or rpm that allows the xen* files to be built on my host, so I'm flying blind w/r/t proper syntax ;-)]
if (virUUIDGenerate(uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID"));
[...]
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 needed changes in callers in order to not leak the returned value are coming in the following patches. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 08fbfff44f..c044cb9672 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -228,23 +228,23 @@ xenConfigGetString(virConfPtr conf, const 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) { + *value = VIR_STRDUP(def); return 0; } - if (val->type != VIR_CONF_STRING) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("config value %s was malformed"), name); - return -1; - } - if (!val->str) - *value = def; + if (!string) + *value = VIR_STRDUP(def); else - *value = val->str; + *value = string; + return 0; } -- 2.17.1
On 09/18/2018 02:48 PM, 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 needed changes in callers in order to not leak the returned
> value are coming in the following patches.
Having a patch with a known memory leak to be fixed by "n" subsequent
patches is in general not accepted. If you didn't know about them (as is
often the case), then we'd be good.
One thing that you "may" consider (and I wasn't involved in) is using
the VIR_AUTOFREE stuff that automagically cleans up for variables. Talk
with Erik or Pavel, they were highly involved.
Of course that assumes you fix a couple other issues - read on...
>
> Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
> ---
> src/xenconfig/xen_common.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 08fbfff44f..c044cb9672 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -228,23 +228,23 @@ xenConfigGetString(virConfPtr conf,
> const char **value,
Being able to assign an allocated buffer to *value leaves me wondering
why the compiler isn't throwing up because the "const"-ness...
> 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) {
> + *value = VIR_STRDUP(def);
I don't think you're compiling this code (like me) since VIR_STRDUP is
generally something like:
if (VIR_STRDUP(*value, def) < 0)
return -1;
and I know for sure the compiler would complain as it would if *value is
a "const char **"... As an example this should throw up with for example
if I added a properly formatted VIR_STRDUP to vsh.c:
In file included from vsh.c:60:
vsh.c: In function 'vshCommandOptStringQuiet':
../src/util/virstring.h:167:41: error: passing argument 1 of 'virStrdup'
from incompatible pointer type [-Werror=incompatible-pointer-types]
# define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \
^~~~~~
vsh.c:1026:9: note: in expansion of macro 'VIR_STRDUP'
if (VIR_STRDUP(*value, "shit this works") < 0)
^~~~~~~~~~
../src/util/virstring.h:137:22: note: expected 'char **' but argument is
of type 'const char **'
int virStrdup(char **dest, const char *src, bool report, int domcode,
~~~~~~~^~~~
Once you get your build working, then I think if you change this *and*
all the callers to use "VIR_AUTOFREE(char *) value = NULL;" (where
@value will be each value that needs to be free'd) and change the
API/prototype to use "char **" instead of "const char **", then I think
all that in one patch will do what you want.
I won't look at patches 4 -> 12 since they're impacted by the above, but
I do note that you missed xenParseCharDev.
John
> return 0;
> }
> > - if (val->type != VIR_CONF_STRING) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("config value %s was malformed"), name);
> - return -1;
> - }
> - if (!val->str)
> - *value = def;
> + if (!string)
> + *value = VIR_STRDUP(def);
> else
> - *value = val->str;
> + *value = string;
> +
> return 0;
> }
>
>
xenConfigGetString returns a newly-allocated pointer and it has to be freed by the caller. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_xl.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 19b6604e05..e4ef061cdb 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -65,37 +65,44 @@ extern int xlu_disk_parse(XLU_Config *cfg, static int xenParseCmdline(virConfPtr conf, char **r_cmdline) { char *cmdline = NULL; - const char *root, *extra, *buf; + const char *root = NULL, *extra = NULL, *buf = NULL; + int ret = -1; if (xenConfigGetString(conf, "cmdline", &buf, NULL) < 0) - return -1; + goto cleanup; if (xenConfigGetString(conf, "root", &root, NULL) < 0) - return -1; + goto cleanup; if (xenConfigGetString(conf, "extra", &extra, NULL) < 0) - return -1; + goto cleanup; if (buf) { if (VIR_STRDUP(cmdline, buf) < 0) - return -1; + goto cleanup; if (root || extra) VIR_WARN("ignoring root= and extra= in favour of cmdline="); } else { if (root && extra) { if (virAsprintf(&cmdline, "root=%s %s", root, extra) < 0) - return -1; + goto cleanup; } else if (root) { if (virAsprintf(&cmdline, "root=%s", root) < 0) - return -1; + goto cleanup; } else if (extra) { if (VIR_STRDUP(cmdline, extra) < 0) - return -1; + goto cleanup; } } *r_cmdline = cmdline; - return 0; + ret = 0; + + cleanup: + VIR_FREE(buf); + VIR_FREE(extra); + VIR_FREE(root); + return ret; } static int -- 2.17.1
xenConfigGetString returns a newly-allocated pointer and it has to be freed by the caller. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_xl.c | 45 ++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index e4ef061cdb..cb1cab8482 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -108,26 +108,28 @@ static int xenParseCmdline(virConfPtr conf, char **r_cmdline) static int xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) { + int ret = -1; size_t i; + const char *bios = NULL; + const char *boot = NULL; + if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { - const char *bios; - const char *boot; int val = 0; if (xenConfigGetString(conf, "bios", &bios, NULL) < 0) - return -1; + goto cleanup; if (bios && STREQ(bios, "ovmf")) { if (VIR_ALLOC(def->os.loader) < 0) - return -1; + goto cleanup; def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH; def->os.loader->readonly = VIR_TRISTATE_BOOL_YES; if (VIR_STRDUP(def->os.loader->path, LIBXL_FIRMWARE_DIR "/ovmf.bin") < 0) - return -1; + goto cleanup; } else { for (i = 0; i < caps->nguests; i++) { if (caps->guests[i]->ostype == VIR_DOMAIN_OSTYPE_HVM && @@ -135,24 +137,24 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) if (VIR_ALLOC(def->os.loader) < 0 || VIR_STRDUP(def->os.loader->path, caps->guests[i]->arch.defaultInfo.loader) < 0) - return -1; + goto cleanup; } } } #ifdef LIBXL_HAVE_BUILDINFO_KERNEL if (xenConfigCopyStringOpt(conf, "kernel", &def->os.kernel) < 0) - return -1; + goto cleanup; if (xenConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) - return -1; + goto cleanup; if (xenParseCmdline(conf, &def->os.cmdline) < 0) - return -1; + goto cleanup; #endif if (xenConfigGetString(conf, "boot", &boot, "c") < 0) - return -1; + goto cleanup; for (i = 0; i < VIR_DOMAIN_BOOT_LAST && boot[i]; i++) { switch (boot[i]) { @@ -174,7 +176,7 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) } if (xenConfigGetBool(conf, "nestedhvm", &val, -1) < 0) - return -1; + goto cleanup; if (val != -1) { const char *vtfeature = "vmx"; @@ -189,7 +191,7 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) if (!def->cpu) { virCPUDefPtr cpu; if (VIR_ALLOC(cpu) < 0) - return -1; + goto cleanup; cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH; cpu->type = VIR_CPU_TYPE_GUEST; @@ -202,26 +204,31 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) if (virCPUDefAddFeature(def->cpu, vtfeature, VIR_CPU_FEATURE_DISABLE) < 0) - return -1; + goto cleanup; } } } else { if (xenConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0) - return -1; + goto cleanup; if (xenConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0) - return -1; + goto cleanup; if (xenConfigCopyStringOpt(conf, "kernel", &def->os.kernel) < 0) - return -1; + goto cleanup; if (xenConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) - return -1; + goto cleanup; if (xenParseCmdline(conf, &def->os.cmdline) < 0) - return -1; + goto cleanup; } - return 0; + ret = 0; + + cleanup: + VIR_FREE(bios); + VIR_FREE(boot); + return ret; } /* -- 2.17.1
xenConfigGetString returns a newly-allocated pointer and it has to be freed by the caller. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_xl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index cb1cab8482..7d86849feb 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -346,6 +346,7 @@ xenParseXLCPUID(virConfPtr conf, virDomainDefPtr def) ret = 0; cleanup: + VIR_FREE(cpuid_str); virStringListFree(name_and_value); virStringListFree(cpuid_pairs); return ret; -- 2.17.1
xenConfigGetString returns a newly-allocated pointer and it has to be freed by the caller. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 41 ++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index c044cb9672..ab4bb7ff3f 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -342,36 +342,43 @@ xenParseTimeOffset(virConfPtr conf, virDomainDefPtr def) static int xenParseEventsActions(virConfPtr conf, virDomainDefPtr def) { - const char *str = NULL; + const char *on_poweroff = NULL, *on_reboot = NULL, *on_crash = NULL; + int ret = -1; - if (xenConfigGetString(conf, "on_poweroff", &str, "destroy") < 0) - return -1; + if (xenConfigGetString(conf, "on_poweroff", &on_poweroff, "destroy") < 0) + goto cleanup; - 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); - return -1; + _("unexpected value %s for on_poweroff"), on_poweroff); + goto cleanup; } - if (xenConfigGetString(conf, "on_reboot", &str, "restart") < 0) - return -1; + if (xenConfigGetString(conf, "on_reboot", &on_reboot, "restart") < 0) + goto cleanup; - 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); - return -1; + _("unexpected value %s for on_reboot"), on_reboot); + goto cleanup; } - if (xenConfigGetString(conf, "on_crash", &str, "restart") < 0) - return -1; + if (xenConfigGetString(conf, "on_crash", &on_crash, "restart") < 0) + goto cleanup; - 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); - return -1; + _("unexpected value %s for on_crash"), on_crash); + goto cleanup; } - return 0; + ret = 0; + + cleanup: + VIR_FREE(on_poweroff); + VIR_FREE(on_reboot); + VIR_FREE(on_crash); + return ret; } -- 2.17.1
xenConfigGetString returns a newly-allocated pointer and it has to be freed by the caller. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 50 +++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index ab4bb7ff3f..f787827008 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -492,40 +492,41 @@ xenParseCPUFeatures(virConfPtr conf, virDomainXMLOptionPtr xmlopt) { unsigned long count = 0; - const char *str = NULL; + const char *cpus = NULL, *tsc_mode = NULL; int val = 0; virDomainTimerDefPtr timer; + int ret = -1; if (xenConfigGetULong(conf, "vcpus", &count, 1) < 0) - return -1; + goto cleanup; if (virDomainDefSetVcpusMax(def, count, xmlopt) < 0) - return -1; + goto cleanup; if (virDomainDefSetVcpus(def, count) < 0) - return -1; + goto cleanup; if (virConfGetValue(conf, "maxvcpus")) { if (xenConfigGetULong(conf, "maxvcpus", &count, 0) < 0) - return -1; + goto cleanup; if (virDomainDefSetVcpusMax(def, count, xmlopt) < 0) - return -1; + goto cleanup; } - if (xenConfigGetString(conf, "cpus", &str, NULL) < 0) - return -1; + if (xenConfigGetString(conf, "cpus", &cpus, NULL) < 0) + goto cleanup; - if (str && (virBitmapParse(str, &def->cpumask, 4096) < 0)) - return -1; + if (cpus && (virBitmapParse(str, &def->cpumask, 4096) < 0)) + goto cleanup; - if (xenConfigGetString(conf, "tsc_mode", &str, NULL) < 0) - return -1; + if (xenConfigGetString(conf, "tsc_mode", &tsc_mode, NULL) < 0) + goto cleanup; - if (str) { + if (tsc_mode) { if (VIR_EXPAND_N(def->clock.timers, def->clock.ntimers, 1) < 0 || VIR_ALLOC(timer) < 0) - return -1; + goto cleanup; timer->name = VIR_DOMAIN_TIMER_NAME_TSC; timer->present = 1; @@ -544,38 +545,38 @@ xenParseCPUFeatures(virConfPtr conf, if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { if (xenConfigGetBool(conf, "pae", &val, 1) < 0) - return -1; + goto cleanup; else if (val) def->features[VIR_DOMAIN_FEATURE_PAE] = VIR_TRISTATE_SWITCH_ON; if (xenConfigGetBool(conf, "acpi", &val, 1) < 0) - return -1; + goto cleanup; else if (val) def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_TRISTATE_SWITCH_ON; if (xenConfigGetBool(conf, "apic", &val, 1) < 0) - return -1; + goto cleanup; else if (val) def->features[VIR_DOMAIN_FEATURE_APIC] = VIR_TRISTATE_SWITCH_ON; if (xenConfigGetBool(conf, "hap", &val, 1) < 0) - return -1; + goto cleanup; else if (!val) def->features[VIR_DOMAIN_FEATURE_HAP] = VIR_TRISTATE_SWITCH_OFF; if (xenConfigGetBool(conf, "viridian", &val, 0) < 0) - return -1; + goto cleanup; else if (val) def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] = VIR_TRISTATE_SWITCH_ON; if (xenConfigGetBool(conf, "hpet", &val, -1) < 0) - return -1; + goto cleanup; if (val != -1) { if (VIR_EXPAND_N(def->clock.timers, def->clock.ntimers, 1) < 0 || VIR_ALLOC(timer) < 0) - return -1; + goto cleanup; timer->name = VIR_DOMAIN_TIMER_NAME_HPET; timer->present = val; @@ -587,7 +588,12 @@ xenParseCPUFeatures(virConfPtr conf, } } - return 0; + ret = 0; + + cleanup: + VIR_FREE(cpus); + VIR_FREE(tsc_mode); + return ret; } -- 2.17.1
xenConfigGetString returns a newly-allocated pointer and it has to be freed by the caller. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index f787827008..5d88577222 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -1060,17 +1060,22 @@ static int xenParseEmulatedDevices(virConfPtr conf, virDomainDefPtr def) { const char *str; + int ret = -1; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { if (xenConfigGetString(conf, "soundhw", &str, NULL) < 0) - return -1; + goto cleanup; if (str && xenParseSxprSound(def, str) < 0) - return -1; + goto cleanup; } - return 0; + ret = 0; + + cleanup: + VIR_FREE(str); + return ret; } -- 2.17.1
xenConfigGetString returns a newly-allocated pointer and it has to be freed by the caller. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 5d88577222..09f93ba449 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -1083,7 +1083,7 @@ static int xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) { virCapsDomainDataPtr capsdata = NULL; - const char *str; + const char *str = NULL; int hvm = 0, ret = -1; if (xenConfigCopyString(conf, "name", &def->name) < 0) @@ -1108,6 +1108,7 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) ret = 0; out: + VIR_FREE(str); VIR_FREE(capsdata); return ret; } -- 2.17.1
xenConfigGetString returns a newly-allocated pointer and it has to be freed by the caller. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_xm.c | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index 7b60f25ec1..a476401183 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -42,16 +42,16 @@ static int xenParseXMOS(virConfPtr conf, virDomainDefPtr def) { size_t i; + int ret = -1; + const char *boot = NULL, *extra = NULL, *root = NULL; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { - const char *boot; - if (VIR_ALLOC(def->os.loader) < 0 || xenConfigCopyString(conf, "kernel", &def->os.loader->path) < 0) - return -1; + goto cleanup; if (xenConfigGetString(conf, "boot", &boot, "c") < 0) - return -1; + goto cleanup; for (i = 0; i < VIR_DOMAIN_BOOT_LAST && boot[i]; i++) { switch (boot[i]) { @@ -72,38 +72,42 @@ xenParseXMOS(virConfPtr conf, virDomainDefPtr def) def->os.nBootDevs++; } } else { - const char *extra, *root; - if (xenConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0) - return -1; + goto cleanup; if (xenConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0) - return -1; + goto cleanup; if (xenConfigCopyStringOpt(conf, "kernel", &def->os.kernel) < 0) - return -1; + goto cleanup; if (xenConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) - return -1; + goto cleanup; if (xenConfigGetString(conf, "extra", &extra, NULL) < 0) - return -1; + goto cleanup; if (xenConfigGetString(conf, "root", &root, NULL) < 0) - return -1; + goto cleanup; if (root && extra) { if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) - return -1; + goto cleanup; } else if (root) { if (virAsprintf(&def->os.cmdline, "root=%s", root) < 0) - return -1; + goto cleanup; } else if (extra) { if (VIR_STRDUP(def->os.cmdline, extra) < 0) - return -1; + goto cleanup; } } - return 0; + ret = 0; + + cleanup: + VIR_FREE(extra); + VIR_FREE(root); + VIR_FREE(boot); + return ret; } -- 2.17.1
xenConfigGetString returns a newly-allocated pointer and it has to be freed by the caller. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_xm.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index a476401183..4bff727e04 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -422,17 +422,18 @@ static int xenParseXMInputDevs(virConfPtr conf, virDomainDefPtr def) { const char *str; + int ret = -1; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { if (xenConfigGetString(conf, "usbdevice", &str, NULL) < 0) - return -1; + goto cleanup; if (str && (STREQ(str, "tablet") || STREQ(str, "mouse") || STREQ(str, "keyboard"))) { virDomainInputDefPtr input; if (VIR_ALLOC(input) < 0) - return -1; + goto cleanup; input->bus = VIR_DOMAIN_INPUT_BUS_USB; if (STREQ(str, "mouse")) @@ -443,11 +444,14 @@ xenParseXMInputDevs(virConfPtr conf, virDomainDefPtr def) input->type = VIR_DOMAIN_INPUT_TYPE_KBD; if (VIR_APPEND_ELEMENT(def->inputs, def->ninputs, input) < 0) { virDomainInputDefFree(input); - return -1; + goto cleanup; } } } - return 0; + ret = 0; + + cleanup: + return ret; } /* -- 2.17.1
The `if (!list || list->type != VIR_CONF_LIST)` couldn't be written in a 100% similar way as `if (virConfGetValueStringList(conf, "pci", false, &pcis) <= 0)` leads us to just ignore any error and return 0 in case of failure. However, for what's needed in this function, this is the closest to the original that we can get and it shouldn't change the function behaviour. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 09f93ba449..9ad081e56b 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -462,27 +462,30 @@ xenParsePCI(char *entry) static int xenParsePCIList(virConfPtr conf, virDomainDefPtr def) { - virConfValuePtr list = virConfGetValue(conf, "pci"); + char **pcis = NULL, **entries; + int ret = -1; - if (!list || list->type != VIR_CONF_LIST) + if (virConfGetValueStringList(conf, "pci", false, &pcis) <= 0) return 0; - for (list = list->list; list; list = list->next) { + for (entries = pcis; *entries; entries++) { + char *entry = *entries; virDomainHostdevDefPtr hostdev; - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - continue; - - if (!(hostdev = xenParsePCI(list->str))) - return -1; + if (!(hostdev = xenParsePCI(entry))) + goto cleanup; if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) { virDomainHostdevDefFree(hostdev); - return -1; + goto cleanup; } } - return 0; + ret = 0; + + cleanup: + virStringListFree(pcis); + return ret; } -- 2.17.1
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
The `if (!list || list->type != VIR_CONF_LIST)` couldn't be written in a 100% similar way as `if (virConfGetValueStringList(conf, "pci", false, &pcis) <= 0)` leads us to just ignore any error and return 0 in case of failure. However, for what's needed in this function, this is the closest to the original that we can get and it shouldn't change the function behaviour.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 09f93ba449..9ad081e56b 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -462,27 +462,30 @@ xenParsePCI(char *entry) static int xenParsePCIList(virConfPtr conf, virDomainDefPtr def) { - virConfValuePtr list = virConfGetValue(conf, "pci"); + char **pcis = NULL, **entries; + int ret = -1;
- if (!list || list->type != VIR_CONF_LIST) + if (virConfGetValueStringList(conf, "pci", false, &pcis) <= 0) return 0;
Since this call passes @false, that means virConfGetValueStringList processing will "fallthrough" the switch for VIR_CONF_STRING and thus generate an error and return -1. So to that degree OK, I agree this is no different than the previous code. I'll point out it's not a great design, but yes, no worse than before. Still if < 0, a virReportError was generated, thus we need to virResetLastError before returning 0 indicating we're going to ignore whether the "pci" value was found and whether it was the right type (e.g. since @false we only want VIR_CONF_LIST values. This will though cause us to miss memory allocation errors, but I have a feeling we'd hit another one real soon. So, this will be ugly and makes assumptions that may not be true in the called function forever, but would look something like this: int rc; if ((rc = virConfGetValueStringList(...)) <= 0) { if (rc < 0) { /* IOW: If called function 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 (virGetLastErrorCode() != VIR_ERR_INTERNAL_ERROR) return -1; /* If we did fall through the switch, then ignore and * clear the last error./ virResetLastError(); } return 0; } See, really ugly and not 100% supportable, but it should cover the existing cases. John
- for (list = list->list; list; list = list->next) { + for (entries = pcis; *entries; entries++) { + char *entry = *entries; virDomainHostdevDefPtr hostdev;
- if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - continue; - - if (!(hostdev = xenParsePCI(list->str))) - return -1; + if (!(hostdev = xenParsePCI(entry))) + goto cleanup;
if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) { virDomainHostdevDefFree(hostdev); - return -1; + goto cleanup; } }
- return 0; + ret = 0; + + cleanup: + virStringListFree(pcis); + return ret; }
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 9ad081e56b..a6e77a9250 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -605,8 +605,10 @@ xenParseCPUFeatures(virConfPtr conf, static int xenParseVfb(virConfPtr conf, virDomainDefPtr def) { + int ret = -1; int val; char *listenAddr = NULL; + char **vfbs = NULL; int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; virConfValuePtr list; virDomainGraphicsDefPtr graphics = NULL; @@ -664,17 +666,14 @@ 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) { + if (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; } @@ -747,12 +746,13 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) } } - return 0; + ret = 0; cleanup: virDomainGraphicsDefFree(graphics); VIR_FREE(listenAddr); - return -1; + virStringListFree(vfbs); + return ret; } -- 2.17.1
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 9ad081e56b..a6e77a9250 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -605,8 +605,10 @@ xenParseCPUFeatures(virConfPtr conf, static int xenParseVfb(virConfPtr conf, virDomainDefPtr def) { + int ret = -1; int val; char *listenAddr = NULL; + char **vfbs = NULL; int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; virConfValuePtr list; virDomainGraphicsDefPtr graphics = NULL; @@ -664,17 +666,14 @@ 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) { + if (virConfGetValueStringList(conf, "vfb", false, &vfbs) == 1) {
There needs to be an else that will do some of the similar checking described in the previous patch... At the very least if return < 0, then virResetLastError... In fact now that I see the same pattern, the code described in the last patch should be a some common local/static function used by the 3 consumers to handle the case where rc is not 1. John
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; }
@@ -747,12 +746,13 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) } }
- return 0; + ret = 0;
cleanup: virDomainGraphicsDefFree(graphics); VIR_FREE(listenAddr); - return -1; + virStringListFree(vfbs); + return ret; }
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index a6e77a9250..786c276c99 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -759,9 +759,11 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) static int xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) { + char **serials = NULL; const char *str; virConfValuePtr value = NULL; virDomainChrDefPtr chr = NULL; + int ret = -1; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { if (xenConfigGetString(conf, "parallel", &str, NULL) < 0) @@ -782,7 +784,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 (virConfGetValueStringList(conf, "serial", false, &serials) == 1) { + char **entries; int portnum = -1; if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) { @@ -791,18 +794,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) goto cleanup; } - value = value->list; - while (value) { - char *port = NULL; + for (entries = serials; *entries; entries++) { + char *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; @@ -827,6 +824,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) chr->target.port = 0; def->serials[0] = chr; def->nserials++; + chr = NULL; } } } else { @@ -840,11 +838,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) def->consoles[0]->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN; } - return 0; + ret = 0; cleanup: virDomainChrDefFree(chr); - return -1; + virStringListFree(serials); + return ret; } -- 2.17.1
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index a6e77a9250..786c276c99 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -759,9 +759,11 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) static int xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) { + char **serials = NULL; const char *str; virConfValuePtr value = NULL; virDomainChrDefPtr chr = NULL; + int ret = -1;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { if (xenConfigGetString(conf, "parallel", &str, NULL) < 0)
Heh heh - this is what I noted earlier - @str isn't going to be FREE'd.
@@ -782,7 +784,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 (virConfGetValueStringList(conf, "serial", false, &serials) == 1) {
Well this is similar to the previous two except that you kept @value here and that line probably should have been deleted as would the value = value->next below and the variable itself as it's unused... At least it shouldn't run off the end of the list...
+ char **entries; int portnum = -1;
if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) { @@ -791,18 +794,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) goto cleanup; }
- value = value->list; - while (value) { - char *port = NULL; + for (entries = serials; *entries; entries++) { + char *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; @@ -827,6 +824,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) chr->target.port = 0; def->serials[0] = chr; def->nserials++; + chr = NULL;
Hmmm... unrelated but equally bad bug. This would require it's own separate patch. John
} } } else { @@ -840,11 +838,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) def->consoles[0]->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN; }
- return 0; + ret = 0;
cleanup: virDomainChrDefFree(chr); - return -1; + virStringListFree(serials); + return ret; }
On Thu, Sep 20, 2018 at 12:12 AM, John Ferlan <jferlan@redhat.com> wrote:
On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/xenconfig/xen_common.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index a6e77a9250..786c276c99 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -759,9 +759,11 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) static int xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) { + char **serials = NULL; const char *str; virConfValuePtr value = NULL; virDomainChrDefPtr chr = NULL; + int ret = -1;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { if (xenConfigGetString(conf, "parallel", &str, NULL) < 0)
Heh heh - this is what I noted earlier - @str isn't going to be FREE'd.
@@ -782,7 +784,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 (virConfGetValueStringList(conf, "serial", false, &serials) == 1) {
Well this is similar to the previous two except that you kept @value here and that line probably should have been deleted as would the value = value->next below and the variable itself as it's unused... At least it shouldn't run off the end of the list...
+ char **entries; int portnum = -1;
if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) { @@ -791,18 +794,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) goto cleanup; }
- value = value->list; - while (value) { - char *port = NULL; + for (entries = serials; *entries; entries++) { + char *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; @@ -827,6 +824,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) chr->target.port = 0; def->serials[0] = chr; def->nserials++; + chr = NULL;
Hmmm... unrelated but equally bad bug. This would require it's own separate patch.
After re-working the patches this change is not needed anymore! \o/
John
} } } else { @@ -840,11 +838,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
def, const char *nativeFormat)
def->consoles[0]->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_
TYPE_XEN;
}
- return 0; + ret = 0;
cleanup: virDomainChrDefFree(chr); - return -1; + virStringListFree(serials); + return ret; }
participants (2)
-
Fabiano Fidêncio -
John Ferlan