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