On Thu, Apr 04, 2013 at 11:56:29AM -0600, Eric Blake wrote:
> On 04/04/2013 07:40 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> >
> > Introduce a method virFileDeleteTree for recursively deleting
> > an entire directory tree
> >
> > Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> > ---
> > 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).
I looked into unlinkat() but that gnulib module is GPL only.
>
> > +
> > + 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().
I also looked at fdopendir() but we'd want gnulib for portability
of that, and again it is GPL only.
> Very weak ACK, depending on what you answer to my commentary.
I would like to make some of the enhancements you suggest, but doing
so would require we implement a bunch of portability code since all
the things you suggest are Linux specific and the gnulib modules are
GPL only. So I think in the immediate term, we'll just have to go
with that I have here.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list