xenParseVfi() does a lot of stuff and, in order to make things cleaner,
let's split it in two new functions:
- xenParseVfi(): it's a new function that keeps the old name. It's
responsible for the whole per-Vfi logic from the old xenParseVfi();
- xenParseVfiList(): it's basically the old xenParsePCI(), but now it
just iterates over the list of Vfis, calling xenParsePCI() per each Vfi.

This patch is basically preparing the ground for the future when
typesafe virConf acessors will be used.

Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
---
 src/xenconfig/xen_common.c | 363 ++++++++++++++++++++++++---------------------
 1 file changed, 192 insertions(+), 171 deletions(-)

diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index fc7b0683b8..45fecbced8 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -846,202 +846,223 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat)
 }
 
 
-static int
-xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename)
+static virDomainNetDefPtr
+xenParseVif(char *entry, const char *vif_typename)
 {
-    char *script = NULL;
     virDomainNetDefPtr net = NULL;
-    virConfValuePtr list = virConfGetValue(conf, "vif");
+    char *script = NULL;
+    char model[10];
+    char type[10];
+    char ip[128];
+    char mac[18];
+    char bridge[50];
+    char vifname[50];
+    char rate[50];
+    char *key;
+    int rc = 0;
+
+    bridge[0] = '\0';
+    mac[0] = '\0';
+    ip[0] = '\0';
+    model[0] = '\0';
+    type[0] = '\0';
+    vifname[0] = '\0';
+    rate[0] = '\0';
+
+    key = entry;
+    while (key) {
+        char *data;
+        char *nextkey = strchr(key, ',');
+
+        if (!(data = strchr(key, '=')))
+            return NULL;
+        data++;
+
+        if (STRPREFIX(key, "mac=")) {
+            int len = nextkey ? (nextkey - data) : sizeof(mac) - 1;
+            if (virStrncpy(mac, data, len, sizeof(mac)) == NULL) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("MAC address %s too big for destination"),
+                               data);
+                return NULL;
+            }
+        } else if (STRPREFIX(key, "bridge=")) {
+            int len = nextkey ? (nextkey - data) : sizeof(bridge) - 1;
+            if (virStrncpy(bridge, data, len, sizeof(bridge)) == NULL) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("Bridge %s too big for destination"),
+                               data);
+                return NULL;
+            }
+        } else if (STRPREFIX(key, "script=")) {
+            int len = nextkey ? (nextkey - data) : strlen(data);
+            VIR_FREE(script);
+            if (VIR_STRNDUP(script, data, len) < 0)
+                return NULL;
+        } else if (STRPREFIX(key, "model=")) {
+            int len = nextkey ? (nextkey - data) : sizeof(model) - 1;
+            if (virStrncpy(model, data, len, sizeof(model)) == NULL) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("Model %s too big for destination"),
+                               data);
+                return NULL;
+            }
+        } else if (STRPREFIX(key, "type=")) {
+            int len = nextkey ? (nextkey - data) : sizeof(type) - 1;
+            if (virStrncpy(type, data, len, sizeof(type)) == NULL) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("Type %s too big for destination"),
+                               data);
+                return NULL;
+            }
+        } else if (STRPREFIX(key, "vifname=")) {
+            int len = nextkey ? (nextkey - data) : sizeof(vifname) - 1;
+            if (virStrncpy(vifname, data, len, sizeof(vifname)) == NULL) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("Vifname %s too big for destination"),
+                               data);
+                return NULL;
+            }
+        } else if (STRPREFIX(key, "ip=")) {
+            int len = nextkey ? (nextkey - data) : sizeof(ip) - 1;
+            if (virStrncpy(ip, data, len, sizeof(ip)) == NULL) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("IP %s too big for destination"), data);
+                return NULL;
+            }
+        } else if (STRPREFIX(key, "rate=")) {
+            int len = nextkey ? (nextkey - data) : sizeof(rate) - 1;
+            if (virStrncpy(rate, data, len, sizeof(rate)) == NULL) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("rate %s too big for destination"), data);
+                return NULL;
+            }
+        }
 
-    if (list && list->type == VIR_CONF_LIST) {
-        list = list->list;
-        while (list) {
-            char model[10];
-            char type[10];
-            char ip[128];
-            char mac[18];
-            char bridge[50];
-            char vifname[50];
-            char rate[50];
-            char *key;
-
-            bridge[0] = '\0';
-            mac[0] = '\0';
-            ip[0] = '\0';
-            model[0] = '\0';
-            type[0] = '\0';
-            vifname[0] = '\0';
-            rate[0] = '\0';
-
-            if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
-                goto skipnic;
-
-            key = list->str;
-            while (key) {
-                char *data;
-                char *nextkey = strchr(key, ',');
+        while (nextkey && (nextkey[0] == ',' ||
+                           nextkey[0] == ' ' ||
+                           nextkey[0] == '\t'))
+            nextkey++;
+        key = nextkey;
+    }
 
-                if (!(data = strchr(key, '=')))
-                    goto skipnic;
-                data++;
-
-                if (STRPREFIX(key, "mac=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(mac) - 1;
-                    if (virStrncpy(mac, data, len, sizeof(mac)) == NULL) {
-                        virReportError(VIR_ERR_INTERNAL_ERROR,
-                                       _("MAC address %s too big for destination"),
-                                       data);
-                        goto skipnic;
-                    }
-                } else if (STRPREFIX(key, "bridge=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(bridge) - 1;
-                    if (virStrncpy(bridge, data, len, sizeof(bridge)) == NULL) {
-                        virReportError(VIR_ERR_INTERNAL_ERROR,
-                                       _("Bridge %s too big for destination"),
-                                       data);
-                        goto skipnic;
-                    }
-                } else if (STRPREFIX(key, "script=")) {
-                    int len = nextkey ? (nextkey - data) : strlen(data);
-                    VIR_FREE(script);
-                    if (VIR_STRNDUP(script, data, len) < 0)
-                        goto cleanup;
-                } else if (STRPREFIX(key, "model=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(model) - 1;
-                    if (virStrncpy(model, data, len, sizeof(model)) == NULL) {
-                        virReportError(VIR_ERR_INTERNAL_ERROR,
-                                       _("Model %s too big for destination"),
-                                       data);
-                        goto skipnic;
-                    }
-                } else if (STRPREFIX(key, "type=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(type) - 1;
-                    if (virStrncpy(type, data, len, sizeof(type)) == NULL) {
-                        virReportError(VIR_ERR_INTERNAL_ERROR,
-                                       _("Type %s too big for destination"),
-                                       data);
-                        goto skipnic;
-                    }
-                } else if (STRPREFIX(key, "vifname=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(vifname) - 1;
-                    if (virStrncpy(vifname, data, len, sizeof(vifname)) == NULL) {
-                        virReportError(VIR_ERR_INTERNAL_ERROR,
-                                       _("Vifname %s too big for destination"),
-                                       data);
-                        goto skipnic;
-                    }
-                } else if (STRPREFIX(key, "ip=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(ip) - 1;
-                    if (virStrncpy(ip, data, len, sizeof(ip)) == NULL) {
-                        virReportError(VIR_ERR_INTERNAL_ERROR,
-                                       _("IP %s too big for destination"), data);
-                        goto skipnic;
-                    }
-                } else if (STRPREFIX(key, "rate=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(rate) - 1;
-                    if (virStrncpy(rate, data, len, sizeof(rate)) == NULL) {
-                        virReportError(VIR_ERR_INTERNAL_ERROR,
-                                       _("rate %s too big for destination"), data);
-                        goto skipnic;
-                    }
-                }
+    if (VIR_ALLOC(net) < 0)
+        goto cleanup;
 
-                while (nextkey && (nextkey[0] == ',' ||
-                                   nextkey[0] == ' ' ||
-                                   nextkey[0] == '\t'))
-                    nextkey++;
-                key = nextkey;
-            }
+    if (mac[0]) {
+        if (virMacAddrParse(mac, &net->mac) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("malformed mac address '%s'"), mac);
+            goto cleanup;
+        }
+    }
 
-            if (VIR_ALLOC(net) < 0)
-                goto cleanup;
+    if (bridge[0] || STREQ_NULLABLE(script, "vif-bridge") ||
+        STREQ_NULLABLE(script, "vif-vnic")) {
+        net->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
+    } else {
+        net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
+    }
 
-            if (mac[0]) {
-                if (virMacAddrParse(mac, &net->mac) < 0) {
-                    virReportError(VIR_ERR_INTERNAL_ERROR,
-                                   _("malformed mac address '%s'"), mac);
-                    goto cleanup;
-                }
-            }
+    if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+        if (bridge[0] && VIR_STRDUP(net->data.bridge.brname, bridge) < 0)
+            goto cleanup;
+    }
+    if (ip[0]) {
+        char **ip_list = virStringSplit(ip, " ", 0);
+        size_t i;
 
-            if (bridge[0] || STREQ_NULLABLE(script, "vif-bridge") ||
-                STREQ_NULLABLE(script, "vif-vnic")) {
-                net->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
-            } else {
-                net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
-            }
+        if (!ip_list)
+            goto cleanup;
 
-            if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
-                if (bridge[0] && VIR_STRDUP(net->data.bridge.brname, bridge) < 0)
-                    goto cleanup;
+        for (i = 0; ip_list[i]; i++) {
+            if (virDomainNetAppendIPAddress(net, ip_list[i], 0, 0) < 0) {
+                virStringListFree(ip_list);
+                goto cleanup;
             }
-            if (ip[0]) {
-                char **ip_list = virStringSplit(ip, " ", 0);
-                size_t i;
+        }
+        virStringListFree(ip_list);
+    }
 
-                if (!ip_list)
-                    goto cleanup;
+    if (script && script[0]) {
+        rc = VIR_STRDUP(net->script, script);
+        if (rc < 0)
+            goto cleanup;
+    }
 
-                for (i = 0; ip_list[i]; i++) {
-                    if (virDomainNetAppendIPAddress(net, ip_list[i], 0, 0) < 0) {
-                        virStringListFree(ip_list);
-                        goto cleanup;
-                    }
-                }
-                virStringListFree(ip_list);
-            }
+    if (model[0]) {
+        rc = VIR_STRDUP(net->model, model);
+        if (rc < 0)
+            goto cleanup;
+    }
 
-            if (script && script[0] &&
-                VIR_STRDUP(net->script, script) < 0)
-                goto cleanup;
+    if (!model[0] && type[0] && STREQ(type, vif_typename)) {
+        rc = VIR_STRDUP(net->model, "netfront");
+        if (rc < 0)
+            goto cleanup;
+    }
 
-            if (model[0] &&
-                VIR_STRDUP(net->model, model) < 0)
-                goto cleanup;
+    if (vifname[0]) {
+        rc = VIR_STRDUP(net->ifname, vifname);
+        if (rc < 0)
+            goto cleanup;
+    }
 
-            if (!model[0] && type[0] && STREQ(type, vif_typename) &&
-                VIR_STRDUP(net->model, "netfront") < 0)
-                goto cleanup;
+    if (rate[0]) {
+        virNetDevBandwidthPtr bandwidth;
+        unsigned long long kbytes_per_sec;
 
-            if (vifname[0] &&
-                VIR_STRDUP(net->ifname, vifname) < 0)
-                goto cleanup;
+        rc = xenParseSxprVifRate(rate, &kbytes_per_sec);
+        if (rc < 0)
+            goto cleanup;
 
-            if (rate[0]) {
-                virNetDevBandwidthPtr bandwidth;
-                unsigned long long kbytes_per_sec;
+        rc = VIR_ALLOC(bandwidth);
+        if (rc < 0)
+            goto cleanup;
+        rc = VIR_ALLOC(bandwidth->out);
+        if (rc < 0) {
+            VIR_FREE(bandwidth);
+            goto cleanup;
+        }
 
-                if (xenParseSxprVifRate(rate, &kbytes_per_sec) < 0)
-                    goto cleanup;
+        bandwidth->out->average = kbytes_per_sec;
+        net->bandwidth = bandwidth;
+    }
 
-                if (VIR_ALLOC(bandwidth) < 0)
-                    goto cleanup;
-                if (VIR_ALLOC(bandwidth->out) < 0) {
-                    VIR_FREE(bandwidth);
-                    goto cleanup;
-                }
+ cleanup:
+    if (rc < 0)
+        virDomainNetDefFree(net);
+    VIR_FREE(script);
+    return net;
+}
 
-                bandwidth->out->average = kbytes_per_sec;
-                net->bandwidth = bandwidth;
-            }
 
-            if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0)
-                goto cleanup;
+static int
+xenParseVifList(virConfPtr conf, virDomainDefPtr def, const char *vif_typename)
+{
+    virConfValuePtr list = virConfGetValue(conf, "vif");
+
+    if (!list || list->type != VIR_CONF_LIST)
+        return 0;
 
-        skipnic:
-            list = list->next;
+    for (list = list->list; list; list = list->next) {
+        virDomainNetDefPtr net = NULL;
+        int rc;
+
+        if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
+            continue;
+
+        if (!(net = xenParseVif(list->str, vif_typename)))
+            return -1;
+
+        rc = VIR_APPEND_ELEMENT(def->nets, def->nnets, net);
+        if (rc < 0) {
             virDomainNetDefFree(net);
-            net = NULL;
-            VIR_FREE(script);
+            return -1;
         }
     }
 
     return 0;
-
- cleanup:
-    virDomainNetDefFree(net);
-    VIR_FREE(script);
-    return -1;
 }
 
 
@@ -1126,10 +1147,10 @@ xenParseConfigCommon(virConfPtr conf,
         return -1;
 
     if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XL)) {
-        if (xenParseVif(conf, def, "vif") < 0)
+        if (xenParseVifList(conf, def, "vif") < 0)
             return -1;
     } else if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) {
-        if (xenParseVif(conf, def, "netfront") < 0)
+        if (xenParseVifList(conf, def, "netfront") < 0)
             return -1;
     } else {
         virReportError(VIR_ERR_INVALID_ARG,
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, May 28, 2018 at 12:28:24AM +0200, Fabiano Fidêncio wrote:
>xenParseVfi() does a lot of stuff and, in order to make things cleaner,
>let's split it in two new functions:
>- xenParseVfi(): it's a new function that keeps the old name. It's
>responsible for the whole per-Vfi logic from the old xenParseVfi();
>- xenParseVfiList(): it's basically the old xenParsePCI(), but now it
>just iterates over the list of Vfis, calling xenParsePCI() per each Vfi.
>

s/Vfi/Vif/g

>This patch is basically preparing the ground for the future when
>typesafe virConf acessors will be used.
>
>Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
>---
> src/xenconfig/xen_common.c | 363 ++++++++++++++++++++++++---------------------
> 1 file changed, 192 insertions(+), 171 deletions(-)
>
>diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
>index fc7b0683b8..45fecbced8 100644
>--- a/src/xenconfig/xen_common.c
>+++ b/src/xenconfig/xen_common.c
>@@ -846,202 +846,223 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat)
> }
>
>
>-static int
>-xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename)
>+static virDomainNetDefPtr
>+xenParseVif(char *entry, const char *vif_typename)
> {
>-    char *script = NULL;
>     virDomainNetDefPtr net = NULL;
>-    virConfValuePtr list = virConfGetValue(conf, "vif");
>+    char *script = NULL;
>+    char model[10];
>+    char type[10];
>+    char ip[128];
>+    char mac[18];
>+    char bridge[50];
>+    char vifname[50];
>+    char rate[50];
>+    char *key;
>+    int rc = 0;

The usage of rc adds a lot of non-whitespace changes.
Instead, you can declare another NetDefPtr:
virDomainNetDefPtr ret = NULL;

>+
>+    bridge[0] = '\0';
>+    mac[0] = '\0';
>+    ip[0] = '\0';
>+    model[0] = '\0';
>+    type[0] = '\0';

[...]

>-                    goto cleanup;
>-                if (VIR_ALLOC(bandwidth->out) < 0) {
>-                    VIR_FREE(bandwidth);
>-                    goto cleanup;
>-                }

do:

VIR_STEAL_PTR(ret, net);

>+ cleanup:
>+    if (rc < 0)
>+        virDomainNetDefFree(net);

And make this free unconditional.

Jano

>+    VIR_FREE(script);
>+    return net;
>+}
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list