8:36 a.m.
The use of virReportError,
On Wed, Jul 16, 2014 at 4:33 PM, David kiarie <davidkiarie4(a)gmail.com> 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(a)suse.com> wrote:
> Jim Fehlig wrote:
>> David Kiarie wrote:
>>
>>> From: Kiarie Kahurani <davidkiarie4(a)gmail.com>
>>>
>>> I introduced the function
>>> xenFormatXMVif(virConfPtr conf, ......);
>>> which parses network configuration
>>>
>>> signed-off-by: David Kiarie <davidkiarie4(a)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;
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?