[libvirt] [PATCH] virsh: Call virDomainFree in cmdDomFSTrim

Peter Krempa pkrempa at redhat.com
Tue Apr 2 15:46:25 UTC 2013


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




More information about the libvir-list mailing list