[...]
>
> > *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).
>
>
>
[...]
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
