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