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

[Eric Blake]

On 04/04/2013 07:40 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> Introduce a method virFileDeleteTree for recursively deleting
> an entire directory tree
> 
> Signed-off-by: Daniel P. Berrange <berrange at 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).

> +
> +        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().

> +
> +        if (S_ISDIR(sb.st_mode)) {
> +            if (virFileDeleteTree(filepath) < 0)
> +                goto cleanup;
> +        } else {
> +            if (unlink(filepath) < 0 && errno != ENOENT) {
> +                virReportSystemError(errno,
> +                                     _("Cannot delete file '%s'"),
> +                                     filepath);
> +                goto cleanup;

What happens on files that have restrictive permissions?  Do we need to
worry about chmod()ing files (or containing directories) to give
ourselves enough access so that we can then turn around and unlink()
files regardless of restrictive permissions?

> +            }
> +        }
> +
> +        VIR_FREE(filepath);
> +        errno = 0;
> +    }
> +
> +    if (errno) {
> +        virReportSystemError(errno, _("Cannot read dir '%s'"),
> +                             dir);
> +        goto cleanup;
> +    }
> +
> +    if (rmdir(dir) < 0 && errno != ENOENT) {
> +        virReportSystemError(errno,
> +                             _("Cannot delete directory '%s'"),
> +                             dir);
> +        goto cleanup;
> +    }

What you have works, even if it is inherently quadratic compared to
using unlinkat().  What sort of trees do we envision deleting?  Do we
need to start worrying about performance, using lessons learned from GNU
coreutils?  Or are the trees small enough, with properties of never
being too-restrictive in file mode, and where the trees we are deleting
are unlikely to be hit by a malicious user exploiting a TOCTTOU race,
that we can just stick with this implementation as-is?

> +
> +    ret = 0;
> +
> +cleanup:
> +    VIR_FREE(filepath);
> +    closedir(dh);
> +    return ret;
> +}
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index c885b73..5f0dd2b 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -108,4 +108,6 @@ int virFileUpdatePerm(const char *path,
>  int virFileLoopDeviceAssociate(const char *file,
>                                 char **dev);
>  
> +int virFileDeleteTree(const char *dir);
> +
>  #endif /* __VIR_FILES_H */

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130404/32797b1f/attachment-0001.sig>