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

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

 



The use of virReportError,

On Wed, Jul 16, 2014 at 4:33 PM, David kiarie <davidkiarie4@xxxxxxxxx> wrote:
> 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]