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