Re: [libvirt] [libvirt PATCH v5 1/1] xen_common: Convert to typesafe virConf acessors
[...]
> *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).
[...]