Re: [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&gt; > --- > 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", &parallel, 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&gt; John