6:17 a.m.
On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:
From: Fabiano Fidêncio <fidencio(a)redhat.com>
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>
---
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