[libvirt] [libvirt PATCH v7 0/6] Finish the conversion to virConfGetValue* functions
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. - 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
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> Reviewed-by: John Ferlan <jferlan@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
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> Reviewed-by: John Ferlan <jferlan@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
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@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
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@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@redhat.com> John
On Thu, Sep 20, 2018 at 10:17 PM, John Ferlan <jferlan@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@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,
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") <
virDomainDefPtr def) 0)
return -1;
- if ((def->onReboot = virDomainLifecycleActionTypeFromString(str))
+ 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) { 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@redhat.com>
John
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@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
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@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@redhat.com> John [...]
On Thu, Sep 20, 2018 at 10:18 PM, John Ferlan <jferlan@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@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!
+ return xenHandleConfGetValueStringListErrors(rc, virGetLastErrorCode());
+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)) <=
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@redhat.com>
John
[...]
Signed-off-by: Fabiano Fidêncio <fidencio@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
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@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@redhat.com> John
+ goto cleanup; } }
On Thu, Sep 20, 2018 at 10:19 PM, John Ferlan <jferlan@redhat.com> wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@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
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote: 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@redhat.com>
Sure!
John
+ goto cleanup; } }
Signed-off-by: Fabiano Fidêncio <fidencio@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
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@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@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;
On Thu, Sep 20, 2018 at 10:20 PM, John Ferlan <jferlan@redhat.com> wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@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
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote: 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@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;
participants (2)
-
Fabiano Fidêncio -
John Ferlan