Re: [libvirt] [PATCH] cgroup: avoid leaking a file
On 05/04/2011 03:35 AM, Daniel Veillard wrote:
On Wed, May 04, 2011 at 12:30:07AM -0400, Laine Stump wrote:
> On 05/03/2011 05:49 PM, Eric Blake wrote:
>> Clang detected a dead store to rc. It turns out that in fixing this,
>> I also found a FILE* leak.
>>
>> * src/util/cgroup.c (virCgroupKillInternal): Abort rather than
>> resuming loop on fscanf failure, and cleanup file on error.
> This definitely fixes the FILE* leak, but do we really want to abort
> the loop (stop looking for more pidfiles) when fscanf fails on one
> pidfile? (dunno, just asking)
[...]
>> @@ -1376,7 +1377,8 @@ static int virCgroupKillInternal(virCgroupPtr group, int
signum, virHashTablePtr
>> if (feof(fp))
>> break;
>> rc = -errno;
>> - break;
>> + VIR_DEBUG("Failed to read %s: %m\n", keypath);
>> + goto cleanup;
>> }
>> if (virHashLookup(pids, (void*)pid))
>> continue;
Seems to me we were doing a break anyway, so it should not change
behaviour there,
The break will get you out of the inner while (!feof(fp)) loop, but if
you managed to get through that inner loop once before failing, and set
done = false, you will then loop again through while (!done) and go to
the next file (if one exists). If you change the break into "goto
cleanup", you will skip any remaining files.
I don't know if that's important or not, but it is a change in behavior.
ACK, but let's have a second patch for after 0.9.1
if we think this need improvement.
Daniel