[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]