Re: [libvirt] [PATCH] virsh: plug memory leaks on failure path

On 03/28/2012 02:58 PM, Alex Jia wrote: > Leaks are introduced in commit 1cf0e3d and fe383bb. > > * tools/virsh.c (cmdSnapshotList): fix memory leaks. > > * How to reproduce? > > % virsh snapshot-list foo --parent --roots > error: --parent and --roots are mutually exclusive > > error: Failed to disconnect from the hypervisor, 1 leaked reference(s) > > % virsh snapshot-list foo --parent --tree > error: --parent and --tree are mutually exclusive > > error: Failed to disconnect from the hypervisor, 1 leaked reference(s) > > % virsh snapshot-list foo --roots --tree > error: --roots and --tree are mutually exclusive > > error: Failed to disconnect from the hypervisor, 1 leaked reference(s) > > ...... > > Signed-off-by: Alex Jia<ajia(a)redhat.com> > --- > tools/virsh.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/tools/virsh.c b/tools/virsh.c > index 5009b6b..f5c3b60 100644 > --- a/tools/virsh.c > +++ b/tools/virsh.c > @@ -16357,11 +16357,13 @@ cmdSnapshotList(vshControl *ctl, const > vshCmd *cmd) > if (vshCommandOptBool(cmd, "roots")) { > vshError(ctl, "%s", > _("--parent and --roots are mutually > exclusive")); > + virDomainFree(dom); > return false; Why not "goto cleanup;" There is a "goto cleanup" right before,

so either change the previous "goto cleanup" into "virDomainFree(dom); return false;" too, or uses the "goto cleanup;" following. > } > if (tree) { > vshError(ctl, "%s", > _("--parent and --tree are mutually exclusive")); > + virDomainFree(dom); > return false; > } > parent_filter = 1; > @@ -16369,11 +16371,13 @@ cmdSnapshotList(vshControl *ctl, const > vshCmd *cmd) > if (tree) { > vshError(ctl, "%s", > _("--roots and --tree are mutually exclusive")); > + virDomainFree(dom); > return false; > } > if (from) { > vshError(ctl, "%s", > _("--roots and --from are mutually exclusive")); > + virDomainFree(dom); This should be a bug, "return" or "goto cleanup;" is missed. But it can be a seperate patch.

> } > flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; > } > @@ -16381,6 +16385,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd > *cmd) > if (tree) { > vshError(ctl, "%s", > _("--leaves and --tree are mutually exclusive")); > + virDomainFree(dom); > return false; > } > flags |= VIR_DOMAIN_SNAPSHOT_LIST_LEAVES; ACK with "goto cleanup;"