[libvirt] [PREPOST 03/17] src/xenxm:Refactor network config parsing code

[Jim Fehlig]

Jim Fehlig wrote:
> David Kiarie wrote:
>   
>> From: Kiarie Kahurani <davidkiarie4 at gmail.com>
>>
>> I introduced the function
>>   xenFormatXMVif(virConfPtr conf, ......);
>> which parses network configuration
>>
>> signed-off-by: David Kiarie <davidkiarie4 at gmail.com>
>> ---
>>  src/xenxs/xen_xm.c | 298 ++++++++++++++++++++++++++++-------------------------
>>  1 file changed, 155 insertions(+), 143 deletions(-)
>>
>> diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
>> index dc840e5..26ebd5a 100644
>> --- a/src/xenxs/xen_xm.c
>> +++ b/src/xenxs/xen_xm.c
>> @@ -309,6 +309,159 @@ static int xenParseXMMem(virConfPtr conf, virDomainDefPtr def)
>>  
>>      return 0;
>>  }
>> +static int xenParseXMVif(virConfPtr conf, virDomainDefPtr def)
>>   
>>     
>
> Same comment here about blank lines between functions.  I won't bore you
> with repeating myself in all the patches.  Also, remember Eric's comment
> about function return type on one line and name and params on another.  
> E.g.
>
> static int
> xenParseXMVif(virConfPtr conf, virDomainDefPtr def)
> {
>     ...
> }
>  
>   
>> +{
>> +    char *script = NULL;
>> +    virDomainNetDefPtr net = NULL;
>> +    virConfValuePtr list = virConfGetValue(conf, "vif");
>>   
>>     
>
> Style is to have a blank line after local variable declarations.
>
> I think you should also define a local variable ret, initialized to -1.
>
>   
>> +    if (list && list->type == VIR_CONF_LIST) {
>> +        list = list->list;
>> +        while (list) {
>> +            char model[10];
>> +            char type[10];
>> +            char ip[16];
>> +            char mac[18];
>> +            char bridge[50];
>> +            char vifname[50];
>> +            char *key;
>> +
>> +            bridge[0] = '\0';
>> +            mac[0] = '\0';
>> +            ip[0] = '\0';
>> +            model[0] = '\0';
>> +            type[0] = '\0';
>> +            vifname[0] = '\0';
>> +
>> +            if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
>> +                goto skipnic;
>> +
>> +            key = list->str;
>> +            while (key) {
>> +                char *data;
>> +                char *nextkey = strchr(key, ',');
>> +
>> +                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;
>> +                    }
>> +                }
>> +
>> +                while (nextkey && (nextkey[0] == ',' ||
>> +                                   nextkey[0] == ' ' ||
>> +                                   nextkey[0] == '\t'))
>> +                    nextkey++;
>> +                key = nextkey;
>> +            }
>> +
>> +            if (VIR_ALLOC(net) < 0)
>> +                goto cleanup;
>> +
>> +            if (mac[0]) {
>> +                if (virMacAddrParse(mac, &net->mac) < 0) {
>> +                    virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                                   _("malformed mac address '%s'"), mac);
>> +                    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 (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>> +                if (bridge[0] && VIR_STRDUP(net->data.bridge.brname, bridge) < 0)
>> +                    goto cleanup;
>> +                if (ip[0] && VIR_STRDUP(net->data.bridge.ipaddr, ip) < 0)
>> +                    goto cleanup;
>> +            } else {
>> +                if (ip[0] && VIR_STRDUP(net->data.ethernet.ipaddr, ip) < 0)
>> +                    goto cleanup;
>> +            }
>> +
>> +            if (script && script[0] &&
>> +                VIR_STRDUP(net->script, script) < 0)
>> +                goto cleanup;
>> +
>> +            if (model[0] &&
>> +                VIR_STRDUP(net->model, model) < 0)
>> +                goto cleanup;
>> +
>> +            if (!model[0] && type[0] && STREQ(type, "netfront") &&
>> +                VIR_STRDUP(net->model, "netfront") < 0)
>> +                goto cleanup;
>> +
>> +            if (vifname[0] &&
>> +                VIR_STRDUP(net->ifname, vifname) < 0)
>> +                goto cleanup;
>> +
>> +            if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0)
>> +                goto cleanup;
>> +
>> +            VIR_FREE(script);
>>   
>>     
>
> Can be freed in cleanup.
>
>   
>> +        skipnic:
>> +            list = list->next;
>> +            virDomainNetDefFree(net);
>>   
>>     
>
> Set 'net = NULL'  now that it has been freed.
>   

Ignore this comment.  It is similar to the comment I made in 7/17, which
Eric clarified

https://www.redhat.com/archives/libvir-list/2014-July/msg00628.html

Regards,
Jim