[libvirt] [libvirt PATCH v5 1/1] xen_common: Convert to typesafe virConf acessors

[John Ferlan]

[...]

> 
>     >          *value = def;
>     > -        return 0;
>     > -    }
>     > -
>     > -    if (val->type != VIR_CONF_STRING) {
>     > -        virReportError(VIR_ERR_INTERNAL_ERROR,
>     > -                       _("config value %s was malformed"), name);
>     >          return -1;
>     >      }
>     > -    if (!val->str)
>     > +    if (!string)
> 
>     and if rc == 1 and !string
> 
>     >          *value = def;
>     >      else
>     > -        *value = val->str;
>     > +        *value = string;
> 
>     And the problem here is that @string would be leaked since it's a
>     VIR_STRDUP of val->str, the @*def is a const char of something.
> 
> 
> Here we can just memcpy() the string content and VIR_FREE() the string.
> Would you agree with this approach or do you have something else in mind?
> 

memcpy @string into what?

Let's look at a caller:

static int
xenParseEventsActions(virConfPtr conf, virDomainDefPtr def)
{
    const char *str = NULL;

    if (xenConfigGetString(conf, "on_poweroff", &str, "destroy") < 0)
        return -1;

So **value is essentially a "const char *str = NULL; with no memory or
stack space to memcpy into. I think we could have a "size" problem with
memcpy into str since it'd need to be variable based upon caller and not
necessarily the same size as we found in the virConfGetValueString.

I don't think this one is going to be "clean" - either you have to keep
using the existing mechanism or have all the callers be able to VIR_FREE
the returned string with of course a VIR_STRDUP(*value, def).

>  
> 
>     >      return 0;
>     >  }
>     >  
>     > @@ -469,27 +446,30 @@ xenParsePCI(char *entry)
>     >  static int
>     >  xenParsePCIList(virConfPtr conf, virDomainDefPtr def)
>     >  {
>     > -    virConfValuePtr list = virConfGetValue(conf, "pci");
>     > +    char **pcis = NULL, **entries;
>     > +    int ret = -1;
>     >  
>     > -    if (!list || list->type != VIR_CONF_LIST)
>     > +    if (virConfGetValueStringList(conf, "pci", false, &pcis) <= 0)
> 
>     Again, not the same checks... Not sure this particular one can be used.
>     The < 0 is an error path...
> 
> 
> I understand your point that we're not doing the same check, but I do
> believe that the "<= 0" check here is okay if we want to keep the same
> behaviour.
> 
> The main issue I see here is that if the list->type is from the wrong
> type, virConfGetValueStringList() would end up returning -1 (considering
> it goes to the default branch, for instance) and the old code would
> return 0 for that case.
> 
> So, < 0 is an error path, but I'm not sure how we can differentiate
> between the errors we want to return 0 and the ones we want to return -1.
>  

I think that became the same conclusion I began reaching, but I was also
trying to keep the non *List context present while reviewing. Once I
came across *List changes and saw some differences, I decided perhaps
it'd be best to punt, ask for the split, and take a fresh look when
round 2 showed up.

Perhaps it's just a process of explaining in the commit or after the ---
why the checks are the same.

John

> 
> 
>     I'm imagining many of the rest are similar.  Probably should have split
>     things up into single patches for each method (as painful as it is) so
>     that it's easier to review and easier to pick and choose what doesn't
>     work and what does.
> 
>     BTW: I would do all the virConfGetValueString's first... Then tackle the
>     virConfGetValueStringList's - since it doesn't cause one to context
>     switch "too much" between two different API's.
> 
> 
> 
> Sure, I'll submit a new version soon (after having your feedback in the
> question above).
>  
> 
> 

[...]