On 03/28/2012 04:12 PM, Osier Yang wrote:
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,
Yeah, I forgot 'ret' has a 'false' initial value.
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.
Yeah, I should include previous change in here.
> }
> 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;"
Thanks,
Alex