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", &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@redhat.com>

John