[libvirt] [libvirt PATCH v6 03/15] xen_common: Change xenConfigGetString to using virConfGetValueString

[John Ferlan]

On 09/18/2018 02:48 PM, 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 needed changes in callers in order to not leak the returned
> value are coming in the following patches.

Having a patch with a known memory leak to be fixed by "n" subsequent
patches is in general not accepted. If you didn't know about them (as is
often the case), then we'd be good.

One thing that you "may" consider (and I wasn't involved in) is using
the VIR_AUTOFREE stuff that automagically cleans up for variables. Talk
with Erik or Pavel, they were highly involved.

Of course that assumes you fix a couple other issues - read on...

> 
> Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> ---
>  src/xenconfig/xen_common.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 08fbfff44f..c044cb9672 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -228,23 +228,23 @@ xenConfigGetString(virConfPtr conf,
>                     const char **value,

Being able to assign an allocated buffer to *value leaves me wondering
why the compiler isn't throwing up because the "const"-ness...

>                     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) {
> +        *value = VIR_STRDUP(def);

I don't think you're compiling this code (like me) since VIR_STRDUP is
generally something like:

    if (VIR_STRDUP(*value, def) < 0)
        return -1;

and I know for sure the compiler would complain as it would if *value is
a "const char **"... As an example this should throw up with for example
if I added a properly formatted VIR_STRDUP to vsh.c:

In file included from vsh.c:60:
vsh.c: In function 'vshCommandOptStringQuiet':
../src/util/virstring.h:167:41: error: passing argument 1 of 'virStrdup'
from incompatible pointer type [-Werror=incompatible-pointer-types]
 # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \
                                         ^~~~~~
vsh.c:1026:9: note: in expansion of macro 'VIR_STRDUP'
     if (VIR_STRDUP(*value, "shit this works") < 0)
         ^~~~~~~~~~
../src/util/virstring.h:137:22: note: expected 'char **' but argument is
of type 'const char **'
 int virStrdup(char **dest, const char *src, bool report, int domcode,
               ~~~~~~~^~~~

Once you get your build working, then I think if you change this *and*
all the callers to use "VIR_AUTOFREE(char *) value = NULL;" (where
@value will be each value that needs to be free'd) and change the
API/prototype to use "char **" instead of "const char **", then I think
all that in one patch will do what you want.

I won't look at patches 4 -> 12 since they're impacted by the above, but
I do note that you missed xenParseCharDev.

John

>          return 0;
>      }
>  > -    if (val->type != VIR_CONF_STRING) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("config value %s was malformed"), name);
> -        return -1;
> -    }
> -    if (!val->str)
> -        *value = def;
> +    if (!string)
> +        *value = VIR_STRDUP(def);
>      else
> -        *value = val->str;
> +        *value = string;
> +
>      return 0;
>  }
>  
>