[libvirt] [PATCH 0/6] Fix some memory leaks and other issues find by coverity tool

[Osier Yang]

On 14/01/14 01:17, John Ferlan wrote:
>
> On 01/13/2014 11:12 AM, Pavel Hrdina wrote:
>> This patch series fixes few memory leaks found by coverity tool
>> to make that tool happy.
>>
>> The last patch is adding only one comment to hide "double_close" error as
>> coverity tool is wrong about this and we don't have to see it in every report.
>> Check the patch for more information.
>>
>> Pavel Hrdina (6):
>>    Fix memory leak in openvz_conf.c
>>    Fix possible memory leak in phyp_driver.c
>>    Fix possible memory leak in util/virxml.c
>>    Fix possible memory leak in virsh-domain-monitor.c
>>    Fix memory leak in securityselinuxlabeltest.c
>>    Fix coverity complain in commandtest.c
>>
>>   src/openvz/openvz_conf.c         | 5 +++--
>>   src/phyp/phyp_driver.c           | 4 +++-
>>   src/util/virxml.c                | 2 ++
>>   tests/commandtest.c              | 1 +
>>   tests/securityselinuxlabeltest.c | 5 ++++-
>>   tools/virsh-domain-monitor.c     | 4 ++++
>>   6 files changed, 17 insertions(+), 4 deletions(-)
>>
> hrmph... for some reason 1/6 and 4/6 haven't made it to my inbox yet.  I
> hope the Red Hat mail filter isn't acting up again...
>
> Anyway
>
> 1/6 is an ACK
>
> 4/6 I think still issues
>
> First off 'type' and 'device' need to be initialized to NULL, then don't
> worry about the "if details()" within the (!target) condition.
>
> Second either we have to choose to exit if the virXPathString() fails
> for type/device or when we go to vshPrint() there needs to be a "type ?
> type : "-"" and "device ? device : "-"" in order to ensure we're not
> printing garbage or old data

Both the "type" and "device" are not mandatory attributes for disk
device, "type" defaults to "file", and "device" defaults to "disk". So in
principle they can't be NULL, if virXPathString returns NULL for them,
it means either the internal data structure is broken (unlikely but
critical), or virXPathString itself had some funny things.  For either
of them, we should fail.  It's unlike "source" element, since the disk
source can be empty, e.g. for a Floppy drive.

>
> Not sure which of the methods is better if we fail to alloc type or
> device is better.  Note that virXPathString() can return NULL for more
> reasons than just allocation failure.
>
> Since Osier wrote the code, maybe he can help decide the course of
> action to take.  Is it better to display "-" or just fail?  On an
> allocation failure, then virXPathString for target will probably fail
> too landing us in that error path, so we could just take that option too!
>

Regards,
Osier