John,
On Fri, Sep 14, 2018 at 5:04 PM, John Ferlan <jfer...@redhat.com> wrote:
>
>
> On 09/11/2018 08:59 AM, Fabiano Fidêncio wrote:
> > There are still a few places using virConfGetValue(), checking for the
> > specific type of the pointers and so on. However, those places are not
> > going to be converted as:
> > - Using virConfGetValue*() would trigger virReportError() in the current
> > code, which would cause, at least, some misleading messages for whoever
> > has to debug this code path;
> > - Expanding virConfValue*() to support strings as other types (for
> > instance, as boolean or long) does not seem to be neither the safest nor
> > the preferential path to take.
> >
> > Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com>
> > ---
> > src/xenconfig/xen_common.c | 163 +++++++++++++++++--------------------
> > 1 file changed, 75 insertions(+), 88 deletions(-)
> >
>
> /me thinks much of the above probably should have gone under the ---
> indicating your knowledge of more places, but the decision to not change
> them for the stated reasons. That commit message doesn't necessarily
> describe the subsequent changes...
>
> Perhaps a generic commit messages such as:
>
> Change to using virConfGetValueString rather than open coding for
> instances in which error messages are expected to be generated.
>
Okay!
>
> > diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> > index fdca9845aa..c31ebfb270 100644
> > --- a/src/xenconfig/xen_common.c
> > +++ b/src/xenconfig/xen_common.c
> > @@ -145,31 +145,19 @@ xenConfigCopyStringInternal(virConfPtr conf,
> > char **value,
> > int allowMissing)
> > {
> > - virConfValuePtr val;
> > + int rc;
> >
> > *value = NULL;
> > - if (!(val = virConfGetValue(conf, name))) {
> > - if (allowMissing)
> > - return 0;
> > - virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("config value %s was missing"), name);
> > - return -1;
> > - }
> > + rc = virConfGetValueString(conf, name, value);
>
> I believe this should be:
>
> if ((rc = virConfGetValueString(conf, name, value)) < 0)
> return -1;
>
> otherwise, we could get to the allowMissing w/ rc == -1 and return 0
> even though previously for a VIR_STRDUP failure we'd return -1. We'd
> also overwrite the no memory error from VIR_STRDUP failure.
>
Fixed locally, thanks!
>
>
> > + if (rc == 1 && *value)
> > + return 0;
> >
> > - if (val->type != VIR_CONF_STRING) {
> > - virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("config value %s was not a string"), name);
> > - return -1;
> > - }
> > - if (!val->str) {
> > - if (allowMissing)
> > - return 0;
> > - virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("config value %s was missing"), name);
> > - return -1;
> > - }
> > + if (allowMissing)
> > + return 0;
> >
> > - return VIR_STRDUP(*value, val->str);
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("config value %s was missing"), name);
> > + return -1;
> > }
> >
> >
> > @@ -193,7 +181,7 @@ xenConfigCopyStringOpt(virConfPtr conf, const char
> *name, char **value)
> > static int
> > xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid)
> > {
> > - virConfValuePtr val;
> > + char *string = NULL;
> >
> > if (!uuid || !name || !conf) {
> > virReportError(VIR_ERR_INVALID_ARG, "%s",
> > @@ -201,7 +189,7 @@ xenConfigGetUUID(virConfPtr conf, const char *name,
> unsigned char *uuid)
> > return -1;
> > }
> >
> > - if (!(val = virConfGetValue(conf, name))) {
> > + if (virConfGetValueString(conf, name, &string) < 0) {
>
> This is not the same check - < 0 is returned on errors (!STRING and
> VIR_STRDUP failure).
>
> I think you should use:
>
> rc = virConfGetValueString(conf, name, &string);
>
> and then continue from there with the rc and/or string checks.
>
Fixed locally, thanks!
>
> > if (virUUIDGenerate(uuid) < 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > "%s", _("Failed to generate UUID"));
> > @@ -211,24 +199,20 @@ xenConfigGetUUID(virConfPtr conf, const char
> *name, unsigned char *uuid)
> > }
> > }
> >
> > - if (val->type != VIR_CONF_STRING) {
> > - virReportError(VIR_ERR_CONF_SYNTAX,
> > - _("config value %s not a string"), name);
> > - return -1;
> > - }
> > -
> > - if (!val->str) {
> > + if (!string) {
>
> I think this would be if virConfGetValueString returns 1, but !string
> because VIR_STRDUP(*value, NULL) would return NULL in *value.
>
Fixed locally, thanks!
>
>
> > virReportError(VIR_ERR_CONF_SYNTAX,
> > _("%s can't be empty"), name);
> > return -1;
> > }
> >
> > - if (virUUIDParse(val->str, uuid) < 0) {
> > + if (virUUIDParse(string, uuid) < 0) {
> > virReportError(VIR_ERR_CONF_SYNTAX,
> > - _("%s not parseable"), val->str);
> > + _("%s not parseable"), string);
> > + VIR_FREE(string);
> > return -1;
> > }
> >
> > + VIR_FREE(string);
> > return 0;
> > }
> >
> > @@ -242,23 +226,16 @@ xenConfigGetString(virConfPtr conf,
> > const char **value,
> > const char *def)
> > {
> > - virConfValuePtr val;
> > + char *string = NULL;
> >
> > - *value = NULL;
> > - if (!(val = virConfGetValue(conf, name))) {
> > + if (virConfGetValueString(conf, name, &string) < 0) {
>
> more of the same here as from previous < 0 is error, while ret == 0
> would be this case
>
Fixed locally, thanks!
>
> > *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?
> > 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'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).
>
> John
>
> > return 0;
> >
> > - for (list = list->list; list; list = list->next) {
> > + for (entries = pcis; *entries; entries++) {
> > + char *entry = *entries;
> > virDomainHostdevDefPtr hostdev;
> >
> > - if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
> > - continue;
> > -
> > - if (!(hostdev = xenParsePCI(list->str)))
> > - return -1;
> > + if (!(hostdev = xenParsePCI(entry)))
> > + goto cleanup;
> >
> > if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev)
> < 0) {
> > virDomainHostdevDefFree(hostdev);
> > - return -1;
> > + goto cleanup;
> > }
> > }
> >
> > - return 0;
> > + ret = 0;
> > +
> > + cleanup:
> > + virStringListFree(pcis);
> > + return ret;
> > }
> >
> >
> > @@ -603,10 +583,11 @@ xenParseCPUFeatures(virConfPtr conf,
> > static int
> > xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> > {
> > + int ret = -1;
> > int val;
> > + char **vfbs = NULL;
> > char *listenAddr = NULL;
> > int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
> > - virConfValuePtr list;
> > virDomainGraphicsDefPtr graphics = NULL;
> >
> > if (hvm) {
> > @@ -662,17 +643,24 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> > }
> >
> > if (!hvm && def->graphics == NULL) { /* New PV guests use this
> format */
> > - list = virConfGetValue(conf, "vfb");
> > - if (list && list->type == VIR_CONF_LIST &&
> > - list->list && list->list->type == VIR_CONF_STRING &&
> > - list->list->str) {
> > + char **entries;
> > + int rc;
> > +
> > + rc = virConfGetValueStringList(conf, "vfb", false, &vfbs);
> > + if (rc <= 0) {
> > + ret = 0;
> > + goto cleanup;
> > + }
> > +
> > + for (entries = vfbs; *entries; entries++) {
> > char vfb[MAX_VFB];
> > char *key = vfb;
> > + char *entry = *entries;
> >
> > - if (virStrcpyStatic(vfb, list->list->str) < 0) {
> > + if (virStrcpyStatic(vfb, entry) < 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("VFB %s too big for destination"),
> > - list->list->str);
> > + entry);
> > goto cleanup;
> > }
> >
> > @@ -745,21 +733,23 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> > }
> > }
> >
> > - return 0;
> > + ret = 0;
> >
> > cleanup:
> > virDomainGraphicsDefFree(graphics);
> > VIR_FREE(listenAddr);
> > - return -1;
> > + virStringListFree(vfbs);
> > + return ret;
> > }
> >
> >
> > static int
> > xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
> *nativeFormat)
> > {
> > + char **serials = NULL;
> > const char *str;
> > - virConfValuePtr value = NULL;
> > virDomainChrDefPtr chr = NULL;
> > + int ret = -1;
> >
> > if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> > if (xenConfigGetString(conf, "parallel", &str, NULL) < 0)
> > @@ -779,8 +769,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
> def, const char *nativeFormat)
> > }
> >
> > /* Try to get the list of values to support multiple serial
> ports */
> > - value = virConfGetValue(conf, "serial");
> > - if (value && value->type == VIR_CONF_LIST) {
> > + if (virConfGetValueStringList(conf, "serial", false, &serials)
> == 1) {
> > + char **entries;
> > int portnum = -1;
> >
> > if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) {
> > @@ -789,18 +779,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
> def, const char *nativeFormat)
> > goto cleanup;
> > }
> >
> > - value = value->list;
> > - while (value) {
> > - char *port = NULL;
> > + for (entries = serials; *entries; entries++) {
> > + char *port = *entries;
> >
> > - if ((value->type != VIR_CONF_STRING) || (value->str ==
> NULL))
> > - goto cleanup;
> > - port = value->str;
> > portnum++;
> > - if (STREQ(port, "none")) {
> > - value = value->next;
> > + if (STREQ(port, "none"))
> > continue;
> > - }
> >
> > if (!(chr = xenParseSxprChar(port, NULL)))
> > goto cleanup;
> > @@ -808,8 +792,6 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
> def, const char *nativeFormat)
> > chr->target.port = portnum;
> > if (VIR_APPEND_ELEMENT(def->serials, def->nserials,
> chr) < 0)
> > goto cleanup;
> > -
> > - value = value->next;
> > }
> > } else {
> > /* If domain is not using multiple serial ports we parse
> data old way */
> > @@ -825,6 +807,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
> def, const char *nativeFormat)
> > chr->target.port = 0;
> > def->serials[0] = chr;
> > def->nserials++;
> > + chr = NULL;
> > }
> > }
> > } else {
> > @@ -838,11 +821,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
> def, const char *nativeFormat)
> > def->consoles[0]->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_
> TYPE_XEN;
> > }
> >
> > - return 0;
> > + ret = 0;
> >
> > cleanup:
> > virDomainChrDefFree(chr);
> > - return -1;
> > + virStringListFree(serials);
> > + return ret;
> > }
> >
> >
> > @@ -1031,29 +1015,32 @@ xenParseVif(char *entry, const char
> *vif_typename)
> > static int
> > xenParseVifList(virConfPtr conf, virDomainDefPtr def, const char
> *vif_typename)
> > {
> > - virConfValuePtr list = virConfGetValue(conf, "vif");
> > + char **vifs = NULL, **entries;
> > + int ret = -1;
> >
> > - if (!list || list->type != VIR_CONF_LIST)
> > + if (virConfGetValueStringList(conf, "vif", false, &vifs) <= 0)
> > return 0;
> >
> > - for (list = list->list; list; list = list->next) {
> > + for (entries = vifs; *entries; entries++) {
> > virDomainNetDefPtr net = NULL;
> > + char *entry = *entries;
> > int rc;
> >
> > - if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
> > - continue;
> > -
> > - if (!(net = xenParseVif(list->str, vif_typename)))
> > - return -1;
> > + if (!(net = xenParseVif(entry, vif_typename)))
> > + goto cleanup;
> >
> > rc = VIR_APPEND_ELEMENT(def->nets, def->nnets, net);
> > if (rc < 0) {
> > virDomainNetDefFree(net);
> > - return -1;
> > + goto cleanup;
> > }
> > }
> >
> > - return 0;
> > + ret = 0;
> > +
> > + cleanup:
> > + virStringListFree(vifs);
> > + return ret;
> > }
> >
> >
> >
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
