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

[Ján Tomko]

On Sat, May 26, 2018 at 11:00:25PM +0200, Fabiano Fidêncio wrote:
>From: Fabiano Fidêncio <fidencio at redhat.com>
>
>Signed-off-by: Fabiano Fidêncio <fabiano at fidencio.org>

The S-o-B address should match the one of the author.

>---
> src/xenconfig/xen_xm.c | 268 ++++++++++++++++++++++++-------------------------
> 1 file changed, 132 insertions(+), 136 deletions(-)
>
>diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c
>index 4becb40b4c..fc88ac8238 100644
>--- a/src/xenconfig/xen_xm.c
>+++ b/src/xenconfig/xen_xm.c
>@@ -112,172 +112,168 @@ xenParseXMDisk(virConfPtr conf, virDomainDefPtr def)
> {
>     virDomainDiskDefPtr disk = NULL;
>     int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
>-    virConfValuePtr list = virConfGetValue(conf, "disk");
>+    char **disks = NULL, **entries;
>

>-    if (list && list->type == VIR_CONF_LIST) {
>-        list = list->list;

Adding
    else
        list = NULL;

Would let you move the while below outside of the if body and separate
the whitespace changes from the virConfGetValue -> StringList change.

Or, even better, the whole per-disk logic can be moved to a separate
function.

>-        while (list) {

>-            char *head;
>-            char *offset;
>-            char *tmp;
>-            const char *src;
>+    if (virConfGetValueStringList(conf, "disk", false, &disks) < 0)
>+        goto cleanup;
>
>-            if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
>-                goto skipdisk;
>+    for (entries = disks; *entries; entries++) {
>+        char *head = *entries;
>+        char *offset;
>+        char *tmp;
>+        const char *src;
>
>-            head = list->str;
>-            if (!(disk = virDomainDiskDefNew(NULL)))
>-                return -1;
>+        if (!(disk = virDomainDiskDefNew(NULL)))
>+            return -1;
>+
>+        /*
>+         * Disks have 3 components, SOURCE,DEST-DEVICE,MODE
>+         * eg, phy:/dev/HostVG/XenGuest1,xvda,w
>+         * The SOURCE is usually prefixed with a driver type,
>+         * and optionally driver sub-type
>+         * The DEST-DEVICE is optionally post-fixed with disk type
>+         */
>+
>+        /* Extract the source file path*/
>+        if (!(offset = strchr(head, ',')))
>+            goto skipdisk;

Using a separate function would also get rid of this unusual label.

>+
>+        if (offset == head) {
>+            /* No source file given, eg CDROM with no media */
>+            ignore_value(virDomainDiskSetSource(disk, NULL));
>+        } else {
>+            if (VIR_STRNDUP(tmp, head, offset - head) < 0)
>+                goto cleanup;
>+
>+            if (virDomainDiskSetSource(disk, tmp) < 0) {
>+                VIR_FREE(tmp);
>+                goto cleanup;
>+            }
>+            VIR_FREE(tmp);
>+        }
>

[...]

>-            skipdisk:
>-            list = list->next;
>-            virDomainDiskDefFree(disk);
>-            disk = NULL;
>-        }
>+        skipdisk:
>+        virDomainDiskDefFree(disk);
>+        disk = NULL;
>     }
>
>     return 0;
>
>  cleanup:
>+    virStringListFree(disks);

The list needs to be freed even in the success path.

(NB: this usage of 'cleanup' label is not customary -
for code paths returning an error, we use 'error'.
'cleanup' is meant for code shared between the success
and error paths)

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/cc5db69e/attachment-0001.sig>