8:56 a.m.
On 19/06/13 21:44, John Ferlan wrote:
On 06/07/2013 01:03 PM, Osier Yang wrote:
> There is POSIX calls to walk through direcotry tree, nftw(3), but
s/is/are
s/direcotry/directory
s/tree/trees or s/tree/a tree/
hm... I reviewed it times before posting it. :(
> there is no way to allow one to pass user data to the callback:
>
> int nftw(const char *dirpath,
> int (*fn) (const char *fpath, const struct stat *sb,
> int typeflag, struct FTW *ftwbuf),
> int nopenfd, int flags);
>
> Assuming a simple requirement: one wants to simply find out all the
> files which have name "vendor" and it has a specific value, under a
> directory, It's not possible to achieve with nftw(3). To solve the
> requirements like this, this new util function comes.
Not sure the last sentence is necessary
> int virTraverseDirectory(const char *dirpath,
> int depth,
> unsigned int flags,
> virTraverseDirectoryCallback cb,
> virTraverseDirectorySkipCallback skipcb,
> void *opaque,
> char ***filepaths);
>
> * Which allow to traverse a directory limited to specified @depth
> (relative to the @dirpath)
>
> * Two callbacks are provided, callback @cb is to handle the file path
> passed to it, with the user data @opaque; callback @skipcb is to
> allow one to make the rules to skip the passed file path. For example,
> one can define regex patterns in @skipcb to skip all the file path
> which matches it.
>
> * @cb should return 0 to indicate the file path is successfully
> handled, otherwise -1 should be returned.
>
> * Like @cb, @skipcb also should return 0 to indiate the file path
> will be skipped to handle, otherwise -1 should be returned.
>
> * If passed @filepath is not NULL, it will be filled with the file
> paths which are successfully handled by @cb.
What use would a NULL filepath be? The whole purpose of the code is to
generate a list of files with a specific pattern. The end result may be
an empty list, but passing NULL
I think there are some use cases won't care about the file paths it
processed, it's a future consideration though, I don't use it in that
way currently.
> * @flags can be used to control the general behaviour of the
> traversing. E.g. Don't follow symbol links.
>
> A simple example:
>
> /*
> * utiltest.c: Prints all the file paths whose basename is
> * "vendor" under directory "./testdir".
> */
> static int
> traverseCallback(const char *path,
> void *opaque)
> {
> const char *name = opaque;
> char *p = NULL;
>
> if ((p = strrchr(path, '/')))
> p++;
>
> if (STRNEQ(p, name))
> return -1;
>
> return 0;
> }
>
> static int
> findVendor(void)
> {
> char **filepaths = NULL;
> unsigned int flags = 0;
> const char *name = "vendor";
> int n;
> int i;
>
> flags |= VIR_TRAVERSE_DIRECTORY_FALL_THROUGH |
> VIR_TRAVERSE_DIRECTORY_IGNORE_HIDDEN_FILES ;
>
> if ((n = virTraverseDirectory("./testdir", 2, flags,
> traverseCallback, NULL,
> (void *)name, &filepaths)) < 0)
> return -1;
>
> for (i = 0; i < n; i++)
> printf("Match%d: %s\n", i, filepaths[i]);
>
> for (i = 0; i < n; i++)
> VIR_FREE(filepaths[i]);
> VIR_FREE(filepaths);
>
> return 0;
> }
>
> Tree view of "./testdir":
> % tree -a ./testdir/
> ./testdir/
> |-- device
> |-- dir1
> | |-- device
> | |-- dir2
> | | |-- device
> | | |-- dir3
> | | | `-- vendor
> | | `-- vendor
> | |-- .dir7
> | | `-- vendor
> | |-- dir8 -> ../dir4
> | `-- vendor
> |-- dir4
> | |-- device
> | |-- dir5
> | | |-- device
> | | `-- vendor
> | `-- vendor
> `-- vendor
>
> 7 directories, 12 files
>
> % ./utiltest
> Match0: ./testdir/dir1/dir2/vendor
> Match1: ./testdir/dir4/dir5/vendor
>
> Only "vendor" in second depth are returned (flag *_FALL_THROUGH took
> effect), and the symbol link is not followed.
>
> "virTraverseDirectory" is implemented using recursion method, which
> is generally not good for performance and memory use, and even may
> eats up the file descriptors. But since we allow to specify the
> tree "depth", and I don't think libvirt has use cases which need to
> traverse a very deep directory tree. And on the other hand,
> Implementing it using either iteration or stack will be much less
> readable.
>
> Later patches will take use of virTraverseDirectory, and tests
> will be added.
> ---
> src/libvirt_private.syms | 1 +
> src/util/virutil.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++-
> src/util/virutil.h | 52 ++++++++++++++++
> 3 files changed, 203 insertions(+), 1 deletion(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 1ea7467..fe182e8 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1965,6 +1965,7 @@ virSetNonBlock;
> virSetUIDGID;
> virSetUIDGIDWithCaps;
> virStrIsPrint;
> +virTraverseDirectory;
> virValidateWWN;
>
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index c5246bc..c1938f9 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -2048,7 +2048,6 @@ virFindFCHostCapableVport(const char *sysfs_prefix
ATTRIBUTE_UNUSED)
> virReportSystemError(ENOSYS, "%s", _("Not supported on this
platform"));
> return NULL;
> }
> -
> #endif /* __linux__ */
>
> /**
> @@ -2070,3 +2069,153 @@ virCompareLimitUlong(unsigned long long a, unsigned long b)
>
> return -1;
> }
> +
> +/*
> + * virTraverseDirectory:
> + * @dirpath: Directory path, (relative or absolute)
> + * @depth: The max directory tree depth for traversing
> + * @cb: Callback to handle file (not directory) during traversing
> + * @skibcb: Callback to define rules to skip entry
s/skibcb/skipcb
> + * @opaque: User data to pass to @cb
> + * @filepaths: Pointer to array to store the file paths, the file
> + * path passed to @cb is stored into @filepaths as long
> + * as @cb returns 0.
> + *
> + * Traverse a directory tree into specified @depth, handing the file
s/handing/handling
> + * with callback in the process. Caller must free @filepaths and
> + * its elements after use.
> + *
> + * Returns the number of file paths successfully handled by @cb on
> + * success, with @fillpaths (if passed @fillpath is not NULL) filled,
s/@fillpaths/@filepaths (there's 2 of them)
> + * or -1 on failure.
> + */
NOTE: If you had run 'make -C tests valgrind' as part of your process
you would have seen there's a memory leak here and the 'utiltest' fails.
The output from my run has the following footprints repeated a few times:
==29216== 8 bytes in 1 blocks are definitely lost in loss record 2 of 39
==29216== at 0x4A0887C: malloc (vg_replace_malloc.c:270)
==29216== by 0x4A089F0: realloc (vg_replace_malloc.c:662)
==29216== by 0x4C6C7EE: virReallocN (viralloc.c:184)
==29216== by 0x4CBC251: virTraverseDirectory (virutil.c:2592)
==29216== by 0x4CBC384: virTraverseDirectory (virutil.c:2562)
==29216== by 0x4CBC384: virTraverseDirectory (virutil.c:2562)
==29216== by 0x4CBCC7D: virFindPCIDeviceByVPD (virutil.c:2084)
==29216== by 0x40202D: testFindPCIDeviceByVPD (utiltest.c:175)
==29216== by 0x40326F: virtTestRun (testutils.c:158)
==29216== by 0x401D20: mymain (utiltest.c:305)
==29216== by 0x4038AA: virtTestMain (testutils.c:722)
==29216== by 0x37C1021A04: (below main) (libc-start.c:225)
==29216==
==29216== 8 bytes in 1 blocks are definitely lost in loss record 3 of 39
==29216== at 0x4A0887C: malloc (vg_replace_malloc.c:270)
==29216== by 0x4A089F0: realloc (vg_replace_malloc.c:662)
==29216== by 0x4C6C7EE: virReallocN (viralloc.c:184)
==29216== by 0x4CBC3B4: virTraverseDirectory (virutil.c:2569)
==29216== by 0x4CBC384: virTraverseDirectory (virutil.c:2562)
==29216== by 0x4CBCC7D: virFindPCIDeviceByVPD (virutil.c:2084)
==29216== by 0x40202D: testFindPCIDeviceByVPD (utiltest.c:175)
==29216== by 0x40326F: virtTestRun (testutils.c:158)
==29216== by 0x401D20: mymain (utiltest.c:305)
==29216== by 0x4038AA: virtTestMain (testutils.c:722)
==29216== by 0x37C1021A04: (below main) (libc-start.c:225)
Beyond that the traversal seems OK to me - I think you're covering the
basics with hidden files and links. I do remember a recent discussion
regarding virFileDeleteTree() which may be useful to revisit to ensure
nothing else is forgotten.
Hm, what virFileDeleteTree does should be able to achieve with
virTraverseDirectory, that's the use case which doesn't have to care
about the returned file paths I think.
> +int
> +virTraverseDirectory(const char *dirpath,
> + int depth,
> + unsigned int flags,
> + virTraverseDirectoryCallback cb,
> + virTraverseDirectorySkipCallback skipcb,
> + void *opaque,
> + char ***filepaths)
> +{
> + struct dirent *entry = NULL;
> + DIR *dir = NULL;
> + char *fpath = NULL;
> + struct stat st;
> + int nfilepaths = 0;
> + int ret = -1;
> + int i;
> +
> + if (depth < 0)
> + return 0;
> +
> + if (filepaths)
> + *filepaths = NULL;
> +
> + if (!(dir = opendir(dirpath))) {
> + virReportSystemError(errno,
> + _("Failed to open directory '%s'"),
> + dirpath);
> + return -1;
> + }
> +
> + while ((entry = readdir(dir))) {
> + /* Always Ignore "." and ".." */
> + if (STREQ(entry->d_name, ".") || STREQ(entry->d_name,
".."))
> + continue;
> +
> + /* Ignore hidden files if it's required */
> + if (flags & VIR_TRAVERSE_DIRECTORY_IGNORE_HIDDEN_FILES &&
> + entry->d_name[0] == '.')
> + continue;
> +
> + if (virAsprintf(&fpath, "%s/%s", dirpath, entry->d_name)
< 0) {
> + virReportOOMError();
> + goto error;
> + }
> +
> + /* Skip to handle the entry as long as @skipcb returns 0 for it */
> + if (skipcb && skipcb(fpath, opaque) == 0) {
> + VIR_FREE(fpath);
> + continue;
> + }
> +
> + if (lstat(fpath, &st) < 0) {
> + virReportSystemError(errno,
> + _("Failed to stat '%s'"),
technically failed to 'lstat'
> + fpath);
> + goto error;
> + }
> +
> + /* Don't follow symbol link unless it's explicitly required */
> + if (S_ISLNK(st.st_mode) &&
> + !(flags & VIR_TRAVERSE_DIRECTORY_FOLLOW_LINK)) {
> + VIR_FREE(fpath);
> + continue;
> + }
> +
> + if (S_ISDIR(st.st_mode)) {
> + char **tmp = NULL;
> + int rc;
> +
> + if ((rc = virTraverseDirectory(fpath, depth - 1, flags, cb, skipcb,
> + opaque, filepaths ? &tmp : NULL))
< 0)
> + goto error;
> +
> + if (rc > 0) {
> + /* Copy the filepaths returned by recursive call */
> + if (filepaths) {
> + if (VIR_REALLOC_N((*filepaths), nfilepaths + rc) < 0) {
> + for (i = 0; i < rc; i++)
> + VIR_FREE(tmp[i]);
> + VIR_FREE(tmp);
> + virReportOOMError();
> + goto error;
> + }
> +
> + for (i = 0; i < rc; i++)
> + (*filepaths)[nfilepaths + i] = tmp[i];
> + }
> + nfilepaths += rc;
> + }
Perhaps the valgrind error is that the else clause here or the one for
"if (filepaths)" in the (rc > 0) condition.
In either case, tmp wouldn't be free()'d properly.
If you required filepaths to be non NULL - I believe the whole
mess/issue would be irrelevant...
> + } else {
> + /* Don't handle the file unless it's the last depth */
> + if ((flags & VIR_TRAVERSE_DIRECTORY_FALL_THROUGH) &&
> + depth != 0) {
> + VIR_FREE(fpath);
> + continue;
> + }
> +
> + if (cb(fpath, opaque) == 0) {
> + if (filepaths) {
> + if (VIR_REALLOC_N((*filepaths), nfilepaths + 1) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> +
> + if (VIR_STRDUP((*filepaths)[nfilepaths], fpath) < 0)
> + goto error;
> +
> + }
> + nfilepaths++;
> + }
> + }
> +
> + VIR_FREE(fpath);
> + }
> +
> + ret = nfilepaths;
> +
> +cleanup:
> + closedir(dir);
> + return ret;
> +
> +error:
> + VIR_FREE(fpath);
> + if (filepaths) {
> + for (i = 0; i < nfilepaths; i++)
> + VIR_FREE((*filepaths)[i]);
> + VIR_FREE(*filepaths);
> + }
> + goto cleanup;
> +}
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index 280a18d..6c46f23 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -166,4 +166,56 @@ char *virFindFCHostCapableVport(const char *sysfs_prefix);
>
> int virCompareLimitUlong(unsigned long long a, unsigned long b);
>
> +/**
> + * virTraverseDirectoryCallback:
> + * @fpath: file path to handle
> + * @opaque: User data to pass to the callback
> + *
> + * Callback for "virTraverseDirectory".
> + *
> + * Returns 0 on success, -1 on failure.
> + */
> +typedef int (*virTraverseDirectoryCallback)(const char *fpath,
> + void *opaque);
> +
> +/**
> + * virTraverseDirectorySkipCallback:
> + * @fpath: file path to handle
> + * @opaque: User data to pass to the callback
> + *
> + * Callback to control whether virTraverseDirectory should ship
> + * @fpath E.g. Skipping files whose name match a regex pattern.
> + *
> + * Return 0 to let "virTraverseDirectory" skip handling the @fpath,
> + * -1 to not skip.
> + */
> +typedef int (*virTraverseDirectorySkipCallback)(const char *fpath,
> + void *opaque);
> +
> +/**
> + * virTraverseDirectoryFlags:
> + *
> + * Except virTraverseDirectorySkipCallback, one can use these flags to
> + * control the general behaviour of virTraverseDirectory
> + */
> +enum {
> + /* Follow symbol links */
> + VIR_TRAVERSE_DIRECTORY_FOLLOW_LINK = 1 << 0,
> +
> + /* Ignore hidden files or directory */
> + VIR_TRAVERSE_DIRECTORY_IGNORE_HIDDEN_FILES = 1 << 1,
> +
> + /* Don't handle files unless it's in the last depth */
> + VIR_TRAVERSE_DIRECTORY_FALL_THROUGH = 1 << 2,
Better named "MATCH_DEPTH_ONLY"? FALL_THROUGH just has other
Not sure, I know FALL_THROUGH is not a good name, but I don't see
MATCH_DEPTH_ONLY makes it better either.. :-), since whether it's
matching or anything else, completely depends on what @cb does.
> +} virTraverseDirectoryFlags;
> +
> +int virTraverseDirectory(const char *dirpath,
> + int depth,
> + unsigned int flags,
> + virTraverseDirectoryCallback cb,
> + virTraverseDirectorySkipCallback skipcb,
> + void *opaque,
> + char ***filepaths);
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)
Not sure "3" is what you mean - perhaps you meant 4 (eg, cb).
yes.. I made too many typos..
It also stands to reason filepaths shouldn't be NULL. I guess I just
don't see the reason to return just a count, but perhaps there's a use
case I'm not thinking about or will be apparent in future patches.
John
> +
> #endif /* __VIR_UTIL_H__ */
>