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

[Alex Jia]

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 at 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