[libvirt] [PATCH] nwfilter: cleanup return codes in nwfilter subsystem

[Stefan Berger]

On 12/08/2011 06:19 PM, Eric Blake wrote:
> On 11/23/2011 02:19 PM, Stefan Berger wrote:
>> This patch cleans up return codes in the nwfilter subsystem.
>> [...]
> Resuming where I left off last time...
>
>> Index: libvirt-acl/src/conf/nwfilter_params.c
>> @@ -505,25 +505,25 @@ virNWFilterHashTablePut(virNWFilterHashT
>>           if (copyName) {
>>               name = strdup(name);
>>               if (!name)
>> -                return 1;
>> +                return -1;
> virNWFilterDetermineMissingVarsRec() has an unchecked call to this
> function, meaning that an OOM error can go undetected in that call site.
>

putting a virReportOOMError() into this one...

> All other calls to this function look sanely converted.
>
>> @@ -640,7 +640,7 @@ virNWFilterHashTablePutAll(virNWFilterHa
>>       return 0;
>>
>>   err_exit:
>> -    return 1;
>> +    return -1;
> All callers look sanely converted.
>
>> Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c
>> ===================================================================
>> --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.c
>> +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c
>> @@ -106,7 +106,7 @@ virNWFilterRuleInstAddData(virNWFilterRu
> Comment prior to the function needs an update.
>

Thanks. Fixed.

>>   {
>>       if (VIR_REALLOC_N(res->data, res->ndata+1)<  0) {
>>           virReportOOMError();
>> -        return 1;
>> +        return -1;
> All callers look sanely converted.
>
>> @@ -151,28 +151,28 @@ virNWFilterVarHashmapAddStdValues(virNWF
>>       if (macaddr) {
>>           val = virNWFilterVarValueCreateSimple(macaddr);
>>           if (!val)
>> -            return 1;
>> +            return -1;
> Comment prior to the function needs an update.  All callers correctly
> converted.

Fixed.
>> @@ -649,7 +649,7 @@ virNWFilterInstantiate(virNWFilterTechDr
>>       virNWFilterHashTablePtr missing_vars = virNWFilterHashTableCreate(0);
>>       if (!missing_vars) {
>>           virReportOOMError();
>> -        rc = 1;
>> +        rc = -1;
> This one calls through a callback that I did not audit:
>          rc = techdriver->applyNewRules(ifname, nptrs, ptrs);
ebiptablesApplyNewRules is being called. The whole ebiptables driver has 
only -1 as failure codes now. So should be ok.

> but assuming the callback is okay (or that you change things to ensure
> rc is -1 even if the callback returns positive), then clients of this
> look okay.
>

>> @@ -792,7 +792,7 @@ __virNWFilterInstantiateFilter(bool tear
>>                                  _("Could not get access to ACL tech "
>>                                  "driver '%s'"),
>>                                  drvname);
>> -        return 1;
>> +        return -1;
> Ultimately called via virNWFilterInstantiateFilterLate, in turn called
> by nwfilter_learnipaddr.c:learnIPAddressThread, which collects the
> return value and prints it in a VIR_DEBUG but does nothing if the value
> was negative.  Is that a problem?
Well, if the instantiation for any reason fails, then the code currently 
'downs' the interface. I didn't know how else it should react since this 
path is invoked in a thread and tearing down the VM would be too severe.
> Also called by the .instantiateFilter callback installed by
> nwfilter_driver.c, which feeds virDomainConfNWFilterInstantiate, and all
> callers look clean.
>
>> @@ -1012,7 +1012,7 @@ int virNWFilterRollbackUpdateFilter(cons
>>                                  _("Could not get access to ACL tech "
>>                                  "driver '%s'"),
>>                                  drvname);
>> -        return 1;
>> +        return -1;
> Should this function be static?  No one outside of gentech_driver calls
> it.  At any rate, callers inside the file are sane.
>

Yes, indeed. I changed it now to be static.

>> @@ -1038,7 +1038,7 @@ virNWFilterTearOldFilter(virDomainNetDef
>>                                  _("Could not get access to ACL tech "
>>                                  "driver '%s'"),
>>                                  drvname);
>> -        return 1;
>> +        return -1;
> Same comments about static, and sane usage.
>
Also changed this one.@@ -530,7 +527,7 @@ learnIPAddressThread(void *arg)
>>           break;
>>       default:
>>           if (techdriver->applyBasicRules(req->ifname,
>> -                                        req->macaddr)) {
>> +                                        req->macaddr)<  0) {
> I didn't audit where this callback came from, but assume it was okay.
>

ebtablesApplyBasicRules now returns -1 in case of failure, so it's ok.
>> @@ -3111,7 +3123,7 @@ ebtablesApplyBasicRules(const char *ifna
>>           virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>                                  _("cannot create rules since ebtables tool is "
>>                                    "missing."));
>> -        return 1;
>> +        return -1;
> Comment before function needs touch up.  Otherwise looks sane.
>
Fixed. Thanks.

>> @@ -4086,7 +4100,7 @@ ebiptablesDriverInit(bool privileged)
>>                                  _("firewall tools were not found or "
>>                                    "cannot be used"));
>>           ebiptablesDriverShutdown();
>> -        return ENOTSUP;
>> +        return -ENOTSUP;
> Looks sane.
>
> Overall, I found one or two nits between my two-stage review, but they
> seemed easy to fix.
>
> ACK.  Up to you if you want to post a delta to this patch for your
> touchups, or just go ahead and commit, since I know you have other
> patches backed up behind this one.
>
I went ahead and pushed the patch. Thanks for this thorough review with 
all the cross-checking and looking through the function descriptions.

Cheers!
  Stefan