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 <fidencio@redhat.com>
> ---
> src/xenconfig/xen_common.c | 163 +++++++++++++++++-------------------- /me thinks much of the above probably should have gone under the ---
> 1 file changed, 75 insertions(+), 88 deletions(-)
>
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.
> 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, I believe this should be:
> 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);
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.
This is not the same check - < 0 is returned on errors (!STRING and
> + 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) {
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.
> if (virUUIDGenerate(uuid) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, I think this would be if virConfGetValueString returns 1, but !string
> "%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) {
because VIR_STRDUP(*value, NULL) would return NULL in *value.
> virReportError(VIR_ERR_CONF_SYNTAX, more of the same here as from previous < 0 is error, while ret == 0
> _("%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) {
would be this case
> *value = def;
> - return 0;
> - }
> -
> - if (val->type != VIR_CONF_STRING) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, and if rc == 1 and !string
> - _("config value %s was malformed"), name);
> return -1;
> }
> - if (!val->str)
> + if (!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.
> 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'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.
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;
> }
>
>
>