On Fri, Sep 14, 2012 at 03:10:13PM +0800, Daniel Veillard wrote:
On Fri, Sep 14, 2012 at 02:24:15PM +0800, Hu Tao wrote:
memset before virResetError will cause memory leak.
virResetError and virCopyError, which calls virResetError, will do memset properly, so we don't have to worry about it here.
Disagree, it's a public API, we can't justify behaviour just on how it is used internally.
NACK, at least the explanation need to be fixed
What is the scenario for the leak ?
The leaked memory was allocated at qemu_monitor.c:636. One of the leak reported by valgrind is: ==12636== 40 bytes in 1 blocks are definitely lost in loss record 302 of 620 ==12636== at 0x4A05E46: malloc (vg_replace_malloc.c:195) ==12636== by 0x306B27FC01: strdup (in /lib64/libc-2.13.so) ==12636== by 0x4EA5669: virCopyError (virterror.c:182) ==12636== by 0x4EA573C: virCopyLastError (virterror.c:282) ==12636== by 0x110CFEA9: qemuMonitorIO (qemu_monitor.c:636) ==12636== by 0x4E83950: virEventPollRunOnce (event_poll.c:485) ==12636== by 0x4E82004: virEventRunDefaultImpl (event.c:247) ==12636== by 0x4F822BC: virNetServerRun (virnetserver.c:751) ==12636== by 0x40C433: main (libvirtd.c:1338) The scenario is: If we deep-copy a virError, by virCopyLastError, into another virError object which is previously deep-copied into, then we have no chance to free previously allocated memory, because the memset in virCopyLastError loses any pointers to them. -- Thanks, Hu Tao