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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list