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

David Kiarie wrote: > From: Kiarie Kahurani <davidkiarie4(a)gmail.com&gt; > > I introduced the function > xenFormatXMVif(virConfPtr conf, ......); > which parses network configuration > > signed-off-by: David Kiarie <davidkiarie4(a)gmail.com&gt; > --- > 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.