Re: [libvirt] [PATCH 03/18] Introduce a virFileDeleteTree method

On 04/04/2013 07:40 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > Introduce a method virFileDeleteTree for recursively deleting > an entire directory tree > > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > --- > src/libvirt_private.syms | 1 + > src/util/virfile.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ > src/util/virfile.h | 2 ++ > 3 files changed, 81 insertions(+) Don't we already have something like this? /me goes and looks... yep, very similar to virCgroupRemoveRecursively - hopefully a later patch drops that function to use this instead. I like the idea of a generalized interface. > + * virFileDeleteTree: > + * > + * Recursively deletes all files / directories > + * starting from the directory @dir. Does not > + * follow symlinks > + */ > +int virFileDeleteTree(const char *dir) > +{ > + DIR *dh = opendir(dir); > + struct dirent *de; > + char *filepath = NULL; > + int ret = -1; > + > + if (!dh) { > + virReportSystemError(errno, _("Cannot open dir '%s'"), > + dir); > + return -1; > + } > + > + errno = 0; > + while ((de = readdir(dh)) != NULL) { > + struct stat sb; > + > + if (STREQ(de->d_name, ".") || > + STREQ(de->d_name, "..")) > + continue; > + > + if (virAsprintf(&filepath, "%s/%s", > + dir, de->d_name) < 0) { > + virReportOOMError(); > + goto cleanup; > + } We should use gnulib's LGPL unlinkat. On capable kernels, it avoids O(n^2) behavior that is inherent in computing filenames in a deep hierarchy. On less capable kernels, it still makes this code simpler to write (no virAsprintf needed here).

> + > + if (lstat(filepath, &sb) < 0) { > + virReportSystemError(errno, _("Cannot access '%s'"), > + filepath); > + goto cleanup; > + } Potentially wasteful on systems like Linux that have d_type. If d_type exists, and is not DT_UNKNOWN, then the difference between DT_DIR and other types can save some system call efforts. Potential TOCTTOU race. POSIX allows unlink("dir") to succeed (although most platforms reject it, either always [Linux], or based on capabilities [Solaris, which also has code to give up that capability, and where gnulib also exposes that]). If we ever manage to unlink() a directory, because we lost the TOCTTOU race, then we have done bad things to the file system. But you are guaranteed that rmdir() on a non-directory will gracefully fail; so you can minimize the race window by attempting to treat _every_ name as a directory first, then gracefully fall back to unlink() if the opendir() fails with ENOTDIR, without ever having to waste time lstat()ing things. Hmm, except that you don't want to follow symlinks, but opendir() follows them by default; so you would have to use open(O_DIRECTORY)/fdopendir() instead of raw opendir().

Very weak ACK, depending on what you answer to my commentary.