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

[Ján Tomko]

On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:
>From: Fabiano Fidêncio <fidencio at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180527/52edb81f/attachment-0001.sig>