On Thu, Sep 20, 2018 at 10:17 PM, John Ferlan <[email protected]> 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 <[email protected]> > > --- > > src/xenconfig/xen_common.c | 84 ++++++++++++++++++++------------------ > > src/xenconfig/xen_common.h | 2 +- > > src/xenconfig/xen_xl.c | 10 +++-- > > src/xenconfig/xen_xm.c | 7 ++-- > > 4 files changed, 56 insertions(+), 47 deletions(-) > > > > diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c > > index 587bab2b19..7b3e5c3b44 100644 > > --- a/src/xenconfig/xen_common.c > > +++ b/src/xenconfig/xen_common.c > > @@ -228,26 +228,28 @@ xenConfigGetUUID(virConfPtr conf, const char > *name, unsigned char *uuid) > > int > > xenConfigGetString(virConfPtr conf, > > const char *name, > > - const char **value, > > + char **value, > > const char *def) > > { > > - virConfValuePtr val; > > + char *string = NULL; > > + int rc; > > > > *value = NULL; > > - if (!(val = virConfGetValue(conf, name))) { > > - *value = def; > > + if ((rc = virConfGetValueString(conf, name, &string)) < 0) > > + return -1; > > + > > + if (rc == 0) { > > + if (VIR_STRDUP(*value, def) < 0) > > + return -1; > > return 0; > > } > > > > - if (val->type != VIR_CONF_STRING) { > > - virReportError(VIR_ERR_INTERNAL_ERROR, > > - _("config value %s was malformed"), name); > > - return -1; > > + if (!string) { > > + if (VIR_STRDUP(*value, def) < 0) > > + return -1; > > + } else { > > + *value = string; > > } > > - if (!val->str) > > - *value = def; > > - else > > - *value = val->str; > > I think this all can be further simplified to: > > if (rc == 0 || !string) { > if (VIR_STRDUP(*value, def) < 0) > return -1; > } else { > *value = string; > } > > return 0; > The only reason I didn't go for it was to have the diff cleaner/smaller. If you're fine merging the "ifs", please, go for it. > > > > return 0; > > } > > > > @@ -345,32 +347,34 @@ xenParseTimeOffset(virConfPtr conf, > virDomainDefPtr def) > > static int > > xenParseEventsActions(virConfPtr conf, virDomainDefPtr def) > > { > > - const char *str = NULL; > > + VIR_AUTOFREE(char *) on_poweroff = NULL; > > + VIR_AUTOFREE(char *) on_reboot = NULL; > > + VIR_AUTOFREE(char *) on_crash = NULL; > > > > - if (xenConfigGetString(conf, "on_poweroff", &str, "destroy") < 0) > > + if (xenConfigGetString(conf, "on_poweroff", &on_poweroff, > "destroy") < 0) > > return -1; > > > > - if ((def->onPoweroff = virDomainLifecycleActionTypeFromString(str)) > < 0) { > > + if ((def->onPoweroff = > > virDomainLifecycleActionTypeFromString(on_poweroff)) > < 0) { > > virReportError(VIR_ERR_INTERNAL_ERROR, > > - _("unexpected value %s for on_poweroff"), str); > > + _("unexpected value %s for on_poweroff"), > on_poweroff); > > return -1; > > } > > > > - if (xenConfigGetString(conf, "on_reboot", &str, "restart") < 0) > > + if (xenConfigGetString(conf, "on_reboot", &on_reboot, "restart") < > 0) > > return -1; > > > > - if ((def->onReboot = virDomainLifecycleActionTypeFromString(str)) > < 0) { > > + if ((def->onReboot = virDomainLifecycleActionTypeFromString(on_reboot)) > < 0) { > > virReportError(VIR_ERR_INTERNAL_ERROR, > > - _("unexpected value %s for on_reboot"), str); > > + _("unexpected value %s for on_reboot"), > on_reboot); > > return -1; > > } > > > > - if (xenConfigGetString(conf, "on_crash", &str, "restart") < 0) > > + if (xenConfigGetString(conf, "on_crash", &on_crash, "restart") < 0) > > return -1; > > > > - if ((def->onCrash = virDomainLifecycleActionTypeFromString(str)) < > 0) { > > + if ((def->onCrash = virDomainLifecycleActionTypeFromString(on_crash)) > < 0) { > > virReportError(VIR_ERR_INTERNAL_ERROR, > > - _("unexpected value %s for on_crash"), str); > > + _("unexpected value %s for on_crash"), on_crash); > > return -1; > > } > > > > @@ -488,7 +492,8 @@ xenParseCPUFeatures(virConfPtr conf, > > virDomainXMLOptionPtr xmlopt) > > { > > unsigned long count = 0; > > - const char *str = NULL; > > + VIR_AUTOFREE(char *) cpus = NULL; > > + VIR_AUTOFREE(char *) tsc_mode = NULL; > > int val = 0; > > virDomainTimerDefPtr timer; > > > > @@ -509,16 +514,16 @@ xenParseCPUFeatures(virConfPtr conf, > > return -1; > > } > > > > - if (xenConfigGetString(conf, "cpus", &str, NULL) < 0) > > + if (xenConfigGetString(conf, "cpus", &cpus, NULL) < 0) > > return -1; > > > > - if (str && (virBitmapParse(str, &def->cpumask, 4096) < 0)) > > + if (cpus && (virBitmapParse(cpus, &def->cpumask, 4096) < 0)) > > return -1; > > > > - if (xenConfigGetString(conf, "tsc_mode", &str, NULL) < 0) > > + if (xenConfigGetString(conf, "tsc_mode", &tsc_mode, NULL) < 0) > > return -1; > > > > - if (str) { > > + if (tsc_mode) { > > if (VIR_EXPAND_N(def->clock.timers, def->clock.ntimers, 1) < 0 > || > > VIR_ALLOC(timer) < 0) > > return -1; > > @@ -528,11 +533,11 @@ xenParseCPUFeatures(virConfPtr conf, > > timer->tickpolicy = -1; > > timer->mode = VIR_DOMAIN_TIMER_MODE_AUTO; > > timer->track = -1; > > - if (STREQ_NULLABLE(str, "always_emulate")) > > + if (STREQ_NULLABLE(tsc_mode, "always_emulate")) > > timer->mode = VIR_DOMAIN_TIMER_MODE_EMULATE; > > - else if (STREQ_NULLABLE(str, "native")) > > + else if (STREQ_NULLABLE(tsc_mode, "native")) > > timer->mode = VIR_DOMAIN_TIMER_MODE_NATIVE; > > - else if (STREQ_NULLABLE(str, "native_paravirt")) > > + else if (STREQ_NULLABLE(tsc_mode, "native_paravirt")) > > Don't believe the _NULLABLE variant is/was required here considering > @str couldn't have been NULL and certainly the right side isn't either! > > Jim must've been super-paranoid for commit b4386fdac or perhaps worried > about the default value of NULL. > > > timer->mode = VIR_DOMAIN_TIMER_MODE_PARAVIRT; > > > > def->clock.timers[def->clock.ntimers - 1] = timer; > > @@ -746,15 +751,15 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) > > static int > > xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char > *nativeFormat) > > { > > - const char *str; > > virConfValuePtr value = NULL; > > virDomainChrDefPtr chr = NULL; > > > > if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { > > - if (xenConfigGetString(conf, "parallel", &str, NULL) < 0) > > + VIR_AUTOFREE(char *) parallel = NULL; > > + if (xenConfigGetString(conf, "parallel", ¶llel, NULL) < 0) > > goto cleanup; > > - if (str && STRNEQ(str, "none") && > > - !(chr = xenParseSxprChar(str, NULL))) > > + if (parallel && STRNEQ(parallel, "none") && > > + !(chr = xenParseSxprChar(parallel, NULL))) > > Then there's this one where STRNEQ_NULLABLE would be the same check, > just like the next one for @serial... > > Again, I won't change - unless you think we should just change these... > > > goto cleanup; > > if (chr) { > > if (VIR_ALLOC_N(def->parallels, 1) < 0) > > @@ -801,11 +806,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr > def, const char *nativeFormat) > > value = value->next; > > } > > } else { > > + VIR_AUTOFREE(char *) serial = NULL; > > /* If domain is not using multiple serial ports we parse > data old way */ > > - if (xenConfigGetString(conf, "serial", &str, NULL) < 0) > > + if (xenConfigGetString(conf, "serial", &serial, NULL) < 0) > > goto cleanup; > > - if (str && STRNEQ(str, "none") && > > - !(chr = xenParseSxprChar(str, NULL))) > > + if (serial && STRNEQ(serial, "none") && > > + !(chr = xenParseSxprChar(serial, NULL))) > > goto cleanup; > > if (chr) { > > if (VIR_ALLOC_N(def->serials, 1) < 0) > > [...] > > I can make the changes locally before pushing - no need for another > series... I'll do the simplification of the logic, but hold off on the > _NULLABLE changes unless you think those are worth changing too. > I totally agree and I don't think the _NULLABLE changes should be part of this commit/series. Please, do the changes! :-) > > Reviewed-by: John Ferlan <[email protected]> > > John >
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
