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

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

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!