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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



I think you comments on PCI parsing code could also apply  here

On Wed, Jul 16, 2014 at 12:19 AM, Jim Fehlig <jfehlig@xxxxxxxx> wrote:
> Jim Fehlig wrote:
>> David Kiarie wrote:
>>
>>> From: Kiarie Kahurani <davidkiarie4@xxxxxxxxx>
>>>
>>> I introduced the function
>>>   xenFormatXMVif(virConfPtr conf, ......);
>>> which parses network configuration
>>>
>>> signed-off-by: David Kiarie <davidkiarie4@xxxxxxxxx>
>>> ---
>>>  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;
here
>>> +                    }
>>> +                } 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;
here
>>> +                    }
>>> +                } 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;
here
>>> +                    }
>>> +                } 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;
here
>>> +                    }
>>> +                } 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;
here
>>> +                    }
>>> +                } 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;
here
>>> +                    }
>>> +                }
>>> +
>>> +                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
>
Should I got ahead and make the changes?

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]