[libvirt] [PATCH] Give virNWFilterVarCombIterNext saner semantics

The virNWFilterVarCombIterNext method will free its parameter when it gets to the end of the iterator. This is somewhat misleading design, making it appear as if the caller has a memory leak. Remove the free'ing of the parameter and ensure that the calling method ebiptablesCreateRuleInstanceIterate free's it instead. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/nwfilter_params.c | 4 +--- src/nwfilter/nwfilter_ebiptables_driver.c | 12 ++++++------ 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index a78c407..5393134 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -526,10 +526,8 @@ next: } } - if (ci->nIter == i) { - virNWFilterVarCombIterFree(ci); + if (ci->nIter == i) return NULL; - } return ci; } diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 0505045..e1e0af8 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2869,14 +2869,14 @@ ebiptablesCreateRuleInstanceIterate( virNWFilterRuleInstPtr res) { int rc = 0; - virNWFilterVarCombIterPtr vciter; + virNWFilterVarCombIterPtr vciter, tmp; /* rule->vars holds all the variables names that this rule will access. * iterate over all combinations of the variables' values and instantiate * the filtering rule with each combination. */ - vciter = virNWFilterVarCombIterCreate(vars, - rule->varAccess, rule->nVarAccess); + tmp = vciter = virNWFilterVarCombIterCreate(vars, + rule->varAccess, rule->nVarAccess); if (!vciter) return -1; @@ -2885,12 +2885,12 @@ ebiptablesCreateRuleInstanceIterate( nwfilter, rule, ifname, - vciter, + tmp, res); if (rc < 0) break; - vciter = virNWFilterVarCombIterNext(vciter); - } while (vciter != NULL); + tmp = virNWFilterVarCombIterNext(tmp); + } while (tmp != NULL); virNWFilterVarCombIterFree(vciter); -- 1.8.5.3

On 17.03.2014 13:34, Daniel P. Berrange wrote:
The virNWFilterVarCombIterNext method will free its parameter when it gets to the end of the iterator. This is somewhat misleading design, making it appear as if the caller has a memory leak. Remove the free'ing of the parameter and ensure that the calling method ebiptablesCreateRuleInstanceIterate free's it instead.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/nwfilter_params.c | 4 +--- src/nwfilter/nwfilter_ebiptables_driver.c | 12 ++++++------ 2 files changed, 7 insertions(+), 9 deletions(-)
ACK Michal

On Mon, Mar 17, 2014 at 12:34:21PM +0000, Daniel P. Berrange wrote:
The virNWFilterVarCombIterNext method will free its parameter when it gets to the end of the iterator. This is somewhat misleading design, making it appear as if the caller has a memory leak. Remove the free'ing of the parameter and ensure that the calling method ebiptablesCreateRuleInstanceIterate free's it instead.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/nwfilter_params.c | 4 +--- src/nwfilter/nwfilter_ebiptables_driver.c | 12 ++++++------ 2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index a78c407..5393134 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -526,10 +526,8 @@ next: } }
- if (ci->nIter == i) { - virNWFilterVarCombIterFree(ci); + if (ci->nIter == i) return NULL; - }
return ci; }
Adding this hunk is indeed needed, I spoke to soon the first time.
diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 0505045..e1e0af8 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2869,14 +2869,14 @@ ebiptablesCreateRuleInstanceIterate( virNWFilterRuleInstPtr res) { int rc = 0; - virNWFilterVarCombIterPtr vciter; + virNWFilterVarCombIterPtr vciter, tmp;
/* rule->vars holds all the variables names that this rule will access. * iterate over all combinations of the variables' values and instantiate * the filtering rule with each combination. */ - vciter = virNWFilterVarCombIterCreate(vars, - rule->varAccess, rule->nVarAccess); + tmp = vciter = virNWFilterVarCombIterCreate(vars, + rule->varAccess, rule->nVarAccess);
But this line should be wrapped, still. ACK to this version with the line wrapped, then. Martin

On 03/17/2014 08:34 AM, Daniel P. Berrange wrote:
The virNWFilterVarCombIterNext method will free its parameter when it gets to the end of the iterator. This is somewhat misleading design, making it appear as if the caller has a memory leak. Remove the free'ing of the parameter and ensure that the calling method ebiptablesCreateRuleInstanceIterate free's it instead.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/nwfilter_params.c | 4 +--- src/nwfilter/nwfilter_ebiptables_driver.c | 12 ++++++------ 2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index a78c407..5393134 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -526,10 +526,8 @@ next: } }
- if (ci->nIter == i) { - virNWFilterVarCombIterFree(ci); + if (ci->nIter == i) return NULL; - }
return ci; } diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 0505045..e1e0af8 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2869,14 +2869,14 @@ ebiptablesCreateRuleInstanceIterate( virNWFilterRuleInstPtr res) { int rc = 0; - virNWFilterVarCombIterPtr vciter; + virNWFilterVarCombIterPtr vciter, tmp;
/* rule->vars holds all the variables names that this rule will access. * iterate over all combinations of the variables' values and instantiate * the filtering rule with each combination. */ - vciter = virNWFilterVarCombIterCreate(vars, - rule->varAccess, rule->nVarAccess); + tmp = vciter = virNWFilterVarCombIterCreate(vars, + rule->varAccess, rule->nVarAccess); if (!vciter) return -1;
@@ -2885,12 +2885,12 @@ ebiptablesCreateRuleInstanceIterate( nwfilter, rule, ifname, - vciter, + tmp, res); if (rc < 0) break; - vciter = virNWFilterVarCombIterNext(vciter); - } while (vciter != NULL); + tmp = virNWFilterVarCombIterNext(tmp); + } while (tmp != NULL);
virNWFilterVarCombIterFree(vciter);
ACK
participants (4)
-
Daniel P. Berrange
-
Martin Kletzander
-
Michal Privoznik
-
Stefan Berger