Re: [libvirt] [PATCHv2 3/3] xen_common: convert to typesafe virConf accessors

On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote: > > From: Fabiano Fidêncio <fidencio(a)redhat.com&gt; > > There are still some places using virConfGetValue() and then checking > the specific type of the pointers and so on. > > Those place are not going to be changed as: > - Directly using virConfGetValue*() would trigger virReportError() on > their current code Is that a problem in xenParseCPUFeatures?

> - Expanding virConfGetValue*() to support strings as other types (as > boolean or long) doesn't seem to be the safest path to take. > > Signed-off-by: Fabiano Fidêncio <fabiano(a)fidencio.org&gt; > --- > src/xenconfig/xen_common.c | 637 > ++++++++++++++++++++++----------------------- > 1 file changed, 307 insertions(+), 330 deletions(-) > > diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c > index a2b0708ee3..2e8e95f720 100644 > --- a/src/xenconfig/xen_common.c > +++ b/src/xenconfig/xen_common.c > @@ -145,31 +145,18 @@ xenConfigCopyStringInternal(virConfPtr conf, > char **value, > int allowMissing) > { > - virConfValuePtr val; > + int result; 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; > - } > > - 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; > - } > + result = virConfGetValueString(conf, name, value); > + if (result == 1 && *value != NULL) > + return 0; > + > + if (allowMissing) > + return 0; > > - return VIR_STRDUP(*value, val->str); > + return -1; > } > > > @@ -193,7 +180,8 @@ xenConfigCopyStringOpt(virConfPtr conf, const char > *name, char **value) > static int > xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) > { > - virConfValuePtr val; > + char *string = NULL; > + int result; int ret = -1;

> > if (!uuid || !name || !conf) { > virReportError(VIR_ERR_INVALID_ARG, "%s", > @@ -201,35 +189,35 @@ xenConfigGetUUID(virConfPtr conf, const char *name, > unsigned char *uuid) > return -1; > } > > - if (!(val = virConfGetValue(conf, name))) { > - if (virUUIDGenerate(uuid) < 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - "%s", _("Failed to generate UUID")); > - return -1; > - } else { > - return 0; > - } > - } > - > - if (val->type != VIR_CONF_STRING) { > - virReportError(VIR_ERR_CONF_SYNTAX, > - _("config value %s not a string"), name); > + if (virConfGetValueString(conf, name, &string) < 0) > return -1; > - } > > - if (!val->str) { > + if (!string) { > virReportError(VIR_ERR_CONF_SYNTAX, > _("%s can't be empty"), name); > return -1; > } > > - if (virUUIDParse(val->str, uuid) < 0) { > + if (virUUIDGenerate(uuid) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("Failed to generate UUID")); > + result = -1; Redundant if you initialize it to -1; > + goto cleanup; > + } > + > + if (virUUIDParse(string, uuid) < 0) { > virReportError(VIR_ERR_CONF_SYNTAX, > - _("%s not parseable"), val->str); > - return -1; > + _("%s not parseable"), string); > + result = -1; Dtto. > + goto cleanup; > } > > - return 0; > + result = 1; ret = 0; > + > + cleanup: > + VIR_FREE(string); > + Dropping this line could improve readability - it's already visually separated by the cleanup label and } > + return result; > } > > > @@ -392,93 +372,92 @@ xenParseEventsActions(virConfPtr conf, > virDomainDefPtr def) > static int > xenParsePCI(virConfPtr conf, virDomainDefPtr def) > { > - virConfValuePtr list = virConfGetValue(conf, "pci"); > virDomainHostdevDefPtr hostdev = NULL; > + char **pcis = NULL, **entries; > Another function that would benefit from being split. [...] > > + virStringListFree(pcis); > return 0; > + > + cleanup: Use the ret variable to join these cleanup paths into one. > + virStringListFree(pcis); > + return -1; > } > > Jano