[libvirt] [PATCH] virsh: Call virDomainFree in cmdDomFSTrim
The virsh domfstrim command was not freeing allocated domain, leaving leaked references behind. --- tools/virsh-domain.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5ddcedc..5fbfeee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + if (dom) + virDomainFree(dom); return ret; } -- 1.8.1.5
On 04/02/2013 09:20 AM, Michal Privoznik wrote:
The virsh domfstrim command was not freeing allocated domain, leaving leaked references behind. --- tools/virsh-domain.c | 2 ++ 1 file changed, 2 insertions(+)
ACK.
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5ddcedc..5fbfeee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) ret = true;
cleanup: + if (dom) + virDomainFree(dom); return ret; }
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/02/2013 05:20 PM, Michal Privoznik wrote:
The virsh domfstrim command was not freeing allocated domain, leaving leaked references behind. --- tools/virsh-domain.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5ddcedc..5fbfeee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) ret = true;
cleanup: + if (dom) + virDomainFree(dom); return ret; }
Alternatively, you could also get out of this with one line less: diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d32218f..99f19a4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10057,7 +10057,7 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false; if (vshCommandOptULongLong(cmd, "minimum", &minimum) < 0) { vshError(ctl, _("Unable to parse integer parameter minimum")); @@ -10075,6 +10075,7 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + virDomainFree(dom); return ret; } -- However, your approach works as well. ACK. Martin ;-)
On 02.04.2013 17:32, Martin Kletzander wrote:
On 04/02/2013 05:20 PM, Michal Privoznik wrote:
The virsh domfstrim command was not freeing allocated domain, leaving leaked references behind. --- tools/virsh-domain.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5ddcedc..5fbfeee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) ret = true;
cleanup: + if (dom) + virDomainFree(dom); return ret; }
Alternatively, you could also get out of this with one line less:
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d32218f..99f19a4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10057,7 +10057,7 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;
In fact, I prefer return ret here; as future changing of return value will require less lines changed and I am used to that. I went with my version. Michal
On 04/02/2013 05:36 PM, Michal Privoznik wrote:
On 02.04.2013 17:32, Martin Kletzander wrote:
On 04/02/2013 05:20 PM, Michal Privoznik wrote:
The virsh domfstrim command was not freeing allocated domain, leaving leaked references behind. --- tools/virsh-domain.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5ddcedc..5fbfeee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) ret = true;
cleanup: + if (dom) + virDomainFree(dom); return ret; }
Alternatively, you could also get out of this with one line less:
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d32218f..99f19a4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10057,7 +10057,7 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;
In fact, I prefer return ret here; as future changing of return value will require less lines changed and I am used to that. I went with my version.
I'd personally prefer virDomainFree() to gracefully handle NULL as parameter, but life just ain't that simple ;-) [JK, there just wasn't much time to enjoy the April Fool's day.] Martin
On 04/02/13 17:40, Martin Kletzander wrote:
On 04/02/2013 05:36 PM, Michal Privoznik wrote:
On 02.04.2013 17:32, Martin Kletzander wrote:
On 04/02/2013 05:20 PM, Michal Privoznik wrote:
The virsh domfstrim command was not freeing allocated domain, leaving leaked references behind. --- tools/virsh-domain.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5ddcedc..5fbfeee 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10058,6 +10058,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) ret = true;
cleanup: + if (dom) + virDomainFree(dom); return ret; }
Alternatively, you could also get out of this with one line less:
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d32218f..99f19a4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10057,7 +10057,7 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;
In fact, I prefer return ret here; as future changing of return value will require less lines changed and I am used to that. I went with my version.
This actually doesn't conform with the rest of the virsh that either uses "goto cleanup" or "return false".
I'd personally prefer virDomainFree() to gracefully handle NULL as parameter, but life just ain't that simple ;-)
I tried that some time ago, wasn't accepted warmly :(
[JK, there just wasn't much time to enjoy the April Fool's day.]
The Czech republic didn't have the chance to but some of the libvirt folks had to work on monday unfortunately :)
Martin
Peter
On 04/02/2013 05:20 PM, Michal Privoznik wrote:
The virsh domfstrim command was not freeing allocated domain, leaving leaked references behind. --- tools/virsh-domain.c | 2 ++ 1 file changed, 2 insertions(+)
You could add the public bugzilla link to the commit message too: https://bugzilla.redhat.com/show_bug.cgi?id=928197 Jan
participants (5)
-
Eric Blake -
Ján Tomko -
Martin Kletzander -
Michal Privoznik -
Peter Krempa