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

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



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 redhat com>
> > 
> > Introduce a method virFileDeleteTree for recursively deleting
> > an entire directory tree
> > 
> > Signed-off-by: Daniel P. Berrange <berrange redhat com>
> > ---
> >  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 :|


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