8:19 p.m.
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