Re: [libvirt] [PATCH v4 2/2] xen_common: convert to typesafe virConf acessors
On Thu, Jun 14, 2018 at 06:59:53AM +0200, Fabiano Fidêncio wrote:
There are still a few places using virConfGetValue(), checking for the
specific type of the pointers and so on.
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 the safest path to
take.
Signed-off-by: Fabiano Fidêncio
---
src/xenconfig/xen_common.c | 197 ++---
1 file changed, 96 insertions(+), 101 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 02765c540b..2dc5e62a9e 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 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;
-}
+rc = virConfGetValueString(conf, name, value);
+if (rc == 1 && *value)
+return 0;
-return VIR_STRDUP(*value, val->str);
+if (allowMissing)
+return 0;
+
Here you return -1 without setting an error.
Previously, "config value %s was missing" was reported.
+return -1;
}
@@ -193,43 +180,43 @@ 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 ret = -1;
if (!uuid || !name || !conf) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
_("Arguments must be non null"));
-return -1;
+goto cleanup;
}
-if (!(val = virConfGetValue(conf, name))) {
+if (virConfGetValueString(conf, name, ) < 0) {
if (virUUIDGenerate(uuid) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("Failed to generate UUID"));
-return -1;
-} else {
-return 0;
+goto cleanup;
}
There are three changes mixed in the diff of this function
* conversion from directly returning -1/0 to using cleanup/ret
* the change of usage val->str to string
* the actual virConfGetValue -> virConfGetValueString change
While the last two somehow belong together, the first one can be
separated to make the logic changes clear. Separating functional
changes makes the commits easier to digest for future readers, see [0]
Previously, if the value was not present, we would attempt to generate
the UUID. After your change, the UUID is generated if something other
than a string was specified, and if the option is missing, an error
is reported.
-}
-if (val->type != VIR_CONF_STRING) {
-virReportError(VIR_ERR_CONF_SYNTAX,
- _("config value %s not a string"), name);
-return -1;
+ret = 0;
+goto cleanup;
}
-if (!val->str) {
+if (!string) {
virReportError(VIR_ERR_CONF_SYNTAX,
_("%s can't be empty"), name);
-return -1;
+goto cleanup;
}
-if (virUUIDParse(val->str, uuid) < 0) {
+if (virUUIDParse(string, uuid) < 0) {
virReportError(VIR_ERR_CONF_SYNTAX,
- _("%s not parseable"), val->str);
-return -1;
+ _("%s not parseable"), string);
+goto cleanup;
}
-return 0;
+ret = 0;
+
+ cleanup:
+VIR_FREE(string);
+return ret;
}
@@ -242,23 +229,16 @@ xenConfigGetString(virConfPtr conf,
const char **value,
const char *def)
{
-virConfValuePtr val;
+char *string = NULL;
-*value = NULL;
-if (!(val = virConfGetValue(conf, name))) {
-*value = def;
-return 0;
-}
+*value = def;
Before, *value was untouched if the config value was malformed.
-if (val->type != VIR_CONF_STRING) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("config value %s was malformed"), name);
+if (virConfGetValueString(conf, name, ) < 0)
return -1;
-}
-if (!val->str)
-*value = def;
-else
-
[libvirt] [PATCH v4 2/2] xen_common: convert to typesafe virConf acessors
There are still a few places using virConfGetValue(), checking for the
specific type of the pointers and so on.
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 the safest path to
take.
Signed-off-by: Fabiano Fidêncio
---
src/xenconfig/xen_common.c | 197 ++---
1 file changed, 96 insertions(+), 101 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 02765c540b..2dc5e62a9e 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 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;
-}
+rc = virConfGetValueString(conf, name, value);
+if (rc == 1 && *value)
+return 0;
-return VIR_STRDUP(*value, val->str);
+if (allowMissing)
+return 0;
+
+return -1;
}
@@ -193,43 +180,43 @@ 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 ret = -1;
if (!uuid || !name || !conf) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
_("Arguments must be non null"));
-return -1;
+goto cleanup;
}
-if (!(val = virConfGetValue(conf, name))) {
+if (virConfGetValueString(conf, name, ) < 0) {
if (virUUIDGenerate(uuid) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("Failed to generate UUID"));
-return -1;
-} else {
-return 0;
+goto cleanup;
}
-}
-if (val->type != VIR_CONF_STRING) {
-virReportError(VIR_ERR_CONF_SYNTAX,
- _("config value %s not a string"), name);
-return -1;
+ret = 0;
+goto cleanup;
}
-if (!val->str) {
+if (!string) {
virReportError(VIR_ERR_CONF_SYNTAX,
_("%s can't be empty"), name);
-return -1;
+goto cleanup;
}
-if (virUUIDParse(val->str, uuid) < 0) {
+if (virUUIDParse(string, uuid) < 0) {
virReportError(VIR_ERR_CONF_SYNTAX,
- _("%s not parseable"), val->str);
-return -1;
+ _("%s not parseable"), string);
+goto cleanup;
}
-return 0;
+ret = 0;
+
+ cleanup:
+VIR_FREE(string);
+return ret;
}
@@ -242,23 +229,16 @@ xenConfigGetString(virConfPtr conf,
const char **value,
const char *def)
{
-virConfValuePtr val;
+char *string = NULL;
-*value = NULL;
-if (!(val = virConfGetValue(conf, name))) {
-*value = def;
-return 0;
-}
+*value = def;
-if (val->type != VIR_CONF_STRING) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("config value %s was malformed"), name);
+if (virConfGetValueString(conf, name, ) < 0)
return -1;
-}
-if (!val->str)
-*value = def;
-else
-*value = val->str;
+
+if (string)
+*value = string;
+
return 0;
}
@@ -469,27 +449,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, ) <= 0)
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)))
+
