2010-11-21 00:42:59

by Shawn Bohrer

Subject: [PATCH] perf: Handle DT_UNKNOWN on filesystems that don't support d_type

Some filesystems like xfs and reiserfs will return DT_UNKNOWN for the
d_type. Handle this case by calling stat() to determine the type.

Signed-off-by: Shawn Bohrer <[email protected]>
---
tools/perf/builtin-trace.c | 35 +++++++++++++++++++++++++++--------
1 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 86cfe38..9cb24fa 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -301,17 +301,36 @@ static int parse_scriptname(const struct option *opt __used,
return 0;
}

-#define for_each_lang(scripts_dir, lang_dirent, lang_next) \
+/* Helper function for filesystems that return a dent->d_type DT_UNKNOWN */
+static int is_directory(const char *base_path, const struct dirent *dent)
+{
+ char path[PATH_MAX];
+ struct stat st;
+
+ sprintf(path, "%s/%s", base_path, dent->d_name);
+ if (stat(path, &st))
+ return 0;
+
+ return S_ISDIR(st.st_mode);
+}
+
+#define for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next)\
while (!readdir_r(scripts_dir, &lang_dirent, &lang_next) && \
lang_next) \
- if (lang_dirent.d_type == DT_DIR && \
+ if (((lang_dirent.d_type == DT_DIR && \
+ lang_dirent.d_type != DT_UNKNOWN) || \
+ (lang_dirent.d_type == DT_UNKNOWN && \
+ is_directory(scripts_path, &lang_dirent))) && \
(strcmp(lang_dirent.d_name, ".")) && \
(strcmp(lang_dirent.d_name, "..")))

-#define for_each_script(lang_dir, script_dirent, script_next) \
+#define for_each_script(lang_path, lang_dir, script_dirent, script_next)\
while (!readdir_r(lang_dir, &script_dirent, &script_next) && \
script_next) \
- if (script_dirent.d_type != DT_DIR)
+ if ((script_dirent.d_type != DT_DIR && \
+ script_dirent.d_type != DT_UNKNOWN) || \
+ (script_dirent.d_type == DT_UNKNOWN && \
+ !is_directory(lang_path, &script_dirent)))


#define RECORD_SUFFIX "-record"
@@ -466,14 +485,14 @@ static int list_available_scripts(const struct option *opt __used,
if (!scripts_dir)
return -1;

- for_each_lang(scripts_dir, lang_dirent, lang_next) {
+ for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {
snprintf(lang_path, MAXPATHLEN, "%s/%s/bin", scripts_path,
lang_dirent.d_name);
lang_dir = opendir(lang_path);
if (!lang_dir)
continue;

- for_each_script(lang_dir, script_dirent, script_next) {
+ for_each_script(lang_path, lang_dir, script_dirent, script_next) {
script_root = strdup(script_dirent.d_name);
str = ends_with(script_root, REPORT_SUFFIX);
if (str) {
@@ -514,14 +533,14 @@ static char *get_script_path(const char *script_root, const char *suffix)
if (!scripts_dir)
return NULL;

- for_each_lang(scripts_dir, lang_dirent, lang_next) {
+ for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {
snprintf(lang_path, MAXPATHLEN, "%s/%s/bin", scripts_path,
lang_dirent.d_name);
lang_dir = opendir(lang_path);
if (!lang_dir)
continue;

- for_each_script(lang_dir, script_dirent, script_next) {
+ for_each_script(lang_path, lang_dir, script_dirent, script_next) {
__script_root = strdup(script_dirent.d_name);
str = ends_with(__script_root, suffix);
if (str) {
--
1.7.2.3

2010-11-21 00:48:43

by Arnaldo Carvalho de Melo

Subject: Re: [PATCH] perf: Handle DT_UNKNOWN on filesystems that don't support d_type

Em Sat, Nov 20, 2010 at 06:42:19PM -0600, Shawn Bohrer escreveu:
> Some filesystems like xfs and reiserfs will return DT_UNKNOWN for the
> d_type. Handle this case by calling stat() to determine the type.

Thanks for the fix, just waiting for some more reviewers to chime in,
seems odd, like readdir_r has a bug.

Even if that is the case we'll have to cope, and doing the extra stat
only when in "doubt" (i.e. when getting DT_UNKNOWN) seems the right
thing to me.

- Arnaldo

> Signed-off-by: Shawn Bohrer <[email protected]>
> ---
> tools/perf/builtin-trace.c | 35 +++++++++++++++++++++++++++--------
> 1 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 86cfe38..9cb24fa 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -301,17 +301,36 @@ static int parse_scriptname(const struct option *opt __used,
> return 0;
> }
>
> -#define for_each_lang(scripts_dir, lang_dirent, lang_next) \
> +/* Helper function for filesystems that return a dent->d_type DT_UNKNOWN */
> +static int is_directory(const char *base_path, const struct dirent *dent)
> +{
> + char path[PATH_MAX];
> + struct stat st;
> +
> + sprintf(path, "%s/%s", base_path, dent->d_name);
> + if (stat(path, &st))
> + return 0;
> +
> + return S_ISDIR(st.st_mode);
> +}
> +
> +#define for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next)\
> while (!readdir_r(scripts_dir, &lang_dirent, &lang_next) && \
> lang_next) \
> - if (lang_dirent.d_type == DT_DIR && \
> + if (((lang_dirent.d_type == DT_DIR && \
> + lang_dirent.d_type != DT_UNKNOWN) || \
> + (lang_dirent.d_type == DT_UNKNOWN && \
> + is_directory(scripts_path, &lang_dirent))) && \
> (strcmp(lang_dirent.d_name, ".")) && \
> (strcmp(lang_dirent.d_name, "..")))
>
> -#define for_each_script(lang_dir, script_dirent, script_next) \
> +#define for_each_script(lang_path, lang_dir, script_dirent, script_next)\
> while (!readdir_r(lang_dir, &script_dirent, &script_next) && \
> script_next) \
> - if (script_dirent.d_type != DT_DIR)
> + if ((script_dirent.d_type != DT_DIR && \
> + script_dirent.d_type != DT_UNKNOWN) || \
> + (script_dirent.d_type == DT_UNKNOWN && \
> + !is_directory(lang_path, &script_dirent)))
>
>
> #define RECORD_SUFFIX "-record"
> @@ -466,14 +485,14 @@ static int list_available_scripts(const struct option *opt __used,
> if (!scripts_dir)
> return -1;
>
> - for_each_lang(scripts_dir, lang_dirent, lang_next) {
> + for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {
> snprintf(lang_path, MAXPATHLEN, "%s/%s/bin", scripts_path,
> lang_dirent.d_name);
> lang_dir = opendir(lang_path);
> if (!lang_dir)
> continue;
>
> - for_each_script(lang_dir, script_dirent, script_next) {
> + for_each_script(lang_path, lang_dir, script_dirent, script_next) {
> script_root = strdup(script_dirent.d_name);
> str = ends_with(script_root, REPORT_SUFFIX);
> if (str) {
> @@ -514,14 +533,14 @@ static char *get_script_path(const char *script_root, const char *suffix)
> if (!scripts_dir)
> return NULL;
>
> - for_each_lang(scripts_dir, lang_dirent, lang_next) {
> + for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {
> snprintf(lang_path, MAXPATHLEN, "%s/%s/bin", scripts_path,
> lang_dirent.d_name);
> lang_dir = opendir(lang_path);
> if (!lang_dir)
> continue;
>
> - for_each_script(lang_dir, script_dirent, script_next) {
> + for_each_script(lang_path, lang_dir, script_dirent, script_next) {
> __script_root = strdup(script_dirent.d_name);
> str = ends_with(__script_root, suffix);
> if (str) {
> --
> 1.7.2.3

2010-11-21 10:01:58

by Andreas Schwab

Subject: Re: [PATCH] perf: Handle DT_UNKNOWN on filesystems that don't support d_type

Shawn Bohrer <[email protected]> writes:

> + if (((lang_dirent.d_type == DT_DIR && \
> + lang_dirent.d_type != DT_UNKNOWN) || \
> + (lang_dirent.d_type == DT_UNKNOWN && \
> + is_directory(scripts_path, &lang_dirent))) && \

if ((lang_dirent.d_type == DT_DIR || \
(lang_dirent.d_type == DT_UNKNOWN && \
is_directory(scripts_path, &lang_dirent))) && \

> + if ((script_dirent.d_type != DT_DIR && \
> + script_dirent.d_type != DT_UNKNOWN) || \
> + (script_dirent.d_type == DT_UNKNOWN && \
> + !is_directory(lang_path, &script_dirent)))

if (script_dirent.d_type != DT_DIR && \
(script_dirent.d_type != DT_UNKNOWN || \
!is_directory(lang_path, &script_dirent)))

(DeMorgan: !(a && b) <-> (!a || !b); !(a || b) <-> (!a && !b))

Andreas.

--
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2010-11-21 15:53:07

by Shawn Bohrer

Subject: Re: [PATCH] perf: Handle DT_UNKNOWN on filesystems that don't support d_type

On Sat, Nov 20, 2010 at 10:48:36PM -0200, Arnaldo Carvalho de Melo wrote:
> Em Sat, Nov 20, 2010 at 06:42:19PM -0600, Shawn Bohrer escreveu:
> > Some filesystems like xfs and reiserfs will return DT_UNKNOWN for the
> > d_type. Handle this case by calling stat() to determine the type.
>
> Thanks for the fix, just waiting for some more reviewers to chime in,
> seems odd, like readdir_r has a bug.

The readdir_r man page says the following:

If the file type could not be determined, the value DT_UNKNOWN
is returned in d_type.

Currently, only some file systems (among them: Btrfs, ext2,
ext3, and ext4) have full support returning the file type in
d_type. All applications must properly handle a return of
DT_UNKNOWN.

So it isn't a bug in readdir_r. This also isn't the only place that
perf uses readdir/readdir_r, and doesn't handle DT_UNKNOWN. In the
other locations it looked to me like it was only reading from debugfs,
so I don't think it matters.

> Even if that is the case we'll have to cope, and doing the extra stat
> only when in "doubt" (i.e. when getting DT_UNKNOWN) seems the right
> thing to me.
>
> - Arnaldo

2010-11-21 15:54:39

by Shawn Bohrer

Subject: Re: [PATCH] perf: Handle DT_UNKNOWN on filesystems that don't support d_type

On Sun, Nov 21, 2010 at 11:01:53AM +0100, Andreas Schwab wrote:
> Shawn Bohrer <[email protected]> writes:
>
> > + if (((lang_dirent.d_type == DT_DIR && \
> > + lang_dirent.d_type != DT_UNKNOWN) || \
> > + (lang_dirent.d_type == DT_UNKNOWN && \
> > + is_directory(scripts_path, &lang_dirent))) && \
>
> if ((lang_dirent.d_type == DT_DIR || \
> (lang_dirent.d_type == DT_UNKNOWN && \
> is_directory(scripts_path, &lang_dirent))) && \
>
> > + if ((script_dirent.d_type != DT_DIR && \
> > + script_dirent.d_type != DT_UNKNOWN) || \
> > + (script_dirent.d_type == DT_UNKNOWN && \
> > + !is_directory(lang_path, &script_dirent)))
>
> if (script_dirent.d_type != DT_DIR && \
> (script_dirent.d_type != DT_UNKNOWN || \
> !is_directory(lang_path, &script_dirent)))
>
> (DeMorgan: !(a && b) <-> (!a || !b); !(a || b) <-> (!a && !b))

Thanks Andreas, I'll send a follow up patch with the cleanup.

--
Shawn

2010-11-21 16:10:21

by Shawn Bohrer

Subject: [PATCH v2] perf: Handle DT_UNKNOWN on filesystems that don't support d_type

Some filesystems like xfs and reiserfs will return DT_UNKNOWN for the
d_type. Handle this case by calling stat() to determine the type.

Signed-off-by: Shawn Bohrer <[email protected]>
---

Cleaned up the boolean logic from Andreas Schwab's review.

tools/perf/builtin-trace.c | 33 +++++++++++++++++++++++++--------
1 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 86cfe38..4783ed8 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -301,17 +301,34 @@ static int parse_scriptname(const struct option *opt __used,
return 0;
}

-#define for_each_lang(scripts_dir, lang_dirent, lang_next) \
+/* Helper function for filesystems that return a dent->d_type DT_UNKNOWN */
+static int is_directory(const char *base_path, const struct dirent *dent)
+{
+ char path[PATH_MAX];
+ struct stat st;
+
+ sprintf(path, "%s/%s", base_path, dent->d_name);
+ if (stat(path, &st))
+ return 0;
+
+ return S_ISDIR(st.st_mode);
+}
+
+#define for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next)\
while (!readdir_r(scripts_dir, &lang_dirent, &lang_next) && \
lang_next) \
- if (lang_dirent.d_type == DT_DIR && \
+ if ((lang_dirent.d_type == DT_DIR || \
+ (lang_dirent.d_type == DT_UNKNOWN && \
+ is_directory(scripts_path, &lang_dirent))) && \
(strcmp(lang_dirent.d_name, ".")) && \
(strcmp(lang_dirent.d_name, "..")))

-#define for_each_script(lang_dir, script_dirent, script_next) \
+#define for_each_script(lang_path, lang_dir, script_dirent, script_next)\
while (!readdir_r(lang_dir, &script_dirent, &script_next) && \
script_next) \
- if (script_dirent.d_type != DT_DIR)
+ if (script_dirent.d_type != DT_DIR && \
+ (script_dirent.d_type != DT_UNKNOWN || \
+ !is_directory(lang_path, &script_dirent)))


#define RECORD_SUFFIX "-record"
@@ -466,14 +483,14 @@ static int list_available_scripts(const struct option *opt __used,
if (!scripts_dir)
return -1;

- for_each_lang(scripts_dir, lang_dirent, lang_next) {
+ for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {
snprintf(lang_path, MAXPATHLEN, "%s/%s/bin", scripts_path,
lang_dirent.d_name);
lang_dir = opendir(lang_path);
if (!lang_dir)
continue;

- for_each_script(lang_dir, script_dirent, script_next) {
+ for_each_script(lang_path, lang_dir, script_dirent, script_next) {
script_root = strdup(script_dirent.d_name);
str = ends_with(script_root, REPORT_SUFFIX);
if (str) {
@@ -514,14 +531,14 @@ static char *get_script_path(const char *script_root, const char *suffix)
if (!scripts_dir)
return NULL;

- for_each_lang(scripts_dir, lang_dirent, lang_next) {
+ for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {
snprintf(lang_path, MAXPATHLEN, "%s/%s/bin", scripts_path,
lang_dirent.d_name);
lang_dir = opendir(lang_path);
if (!lang_dir)
continue;

- for_each_script(lang_dir, script_dirent, script_next) {
+ for_each_script(lang_path, lang_dir, script_dirent, script_next) {
__script_root = strdup(script_dirent.d_name);
str = ends_with(__script_root, suffix);
if (str) {
--
1.7.2.3

2010-11-28 08:35:38

by Shawn Bohrer

Subject: [tip:perf/core] perf trace: Handle DT_UNKNOWN on filesystems that don't support d_type

Commit-ID: 008f29d3865828bb27e35d6d3fa889d0853b469f
Gitweb: http://git.kernel.org/tip/008f29d3865828bb27e35d6d3fa889d0853b469f
Author: Shawn Bohrer <[email protected]>
AuthorDate: Sun, 21 Nov 2010 10:09:39 -0600
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Sat, 27 Nov 2010 01:33:04 -0200

perf trace: Handle DT_UNKNOWN on filesystems that don't support d_type

Some filesystems like xfs and reiserfs will return DT_UNKNOWN for the
d_type. Handle this case by calling stat() to determine the type.

Cc: Andreas Schwab <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Shawn Bohrer <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-trace.c | 33 +++++++++++++++++++++++++--------
1 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 86cfe38..4783ed8 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -301,17 +301,34 @@ static int parse_scriptname(const struct option *opt __used,
return 0;
}

-#define for_each_lang(scripts_dir, lang_dirent, lang_next) \
+/* Helper function for filesystems that return a dent->d_type DT_UNKNOWN */
+static int is_directory(const char *base_path, const struct dirent *dent)
+{
+ char path[PATH_MAX];
+ struct stat st;
+
+ sprintf(path, "%s/%s", base_path, dent->d_name);
+ if (stat(path, &st))
+ return 0;
+
+ return S_ISDIR(st.st_mode);
+}
+
+#define for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next)\
while (!readdir_r(scripts_dir, &lang_dirent, &lang_next) && \
lang_next) \
- if (lang_dirent.d_type == DT_DIR && \
+ if ((lang_dirent.d_type == DT_DIR || \
+ (lang_dirent.d_type == DT_UNKNOWN && \
+ is_directory(scripts_path, &lang_dirent))) && \
(strcmp(lang_dirent.d_name, ".")) && \
(strcmp(lang_dirent.d_name, "..")))

-#define for_each_script(lang_dir, script_dirent, script_next) \
+#define for_each_script(lang_path, lang_dir, script_dirent, script_next)\
while (!readdir_r(lang_dir, &script_dirent, &script_next) && \
script_next) \
- if (script_dirent.d_type != DT_DIR)
+ if (script_dirent.d_type != DT_DIR && \
+ (script_dirent.d_type != DT_UNKNOWN || \
+ !is_directory(lang_path, &script_dirent)))


#define RECORD_SUFFIX "-record"
@@ -466,14 +483,14 @@ static int list_available_scripts(const struct option *opt __used,
if (!scripts_dir)
return -1;

- for_each_lang(scripts_dir, lang_dirent, lang_next) {
+ for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {
snprintf(lang_path, MAXPATHLEN, "%s/%s/bin", scripts_path,
lang_dirent.d_name);
lang_dir = opendir(lang_path);
if (!lang_dir)
continue;

- for_each_script(lang_dir, script_dirent, script_next) {
+ for_each_script(lang_path, lang_dir, script_dirent, script_next) {
script_root = strdup(script_dirent.d_name);
str = ends_with(script_root, REPORT_SUFFIX);
if (str) {
@@ -514,14 +531,14 @@ static char *get_script_path(const char *script_root, const char *suffix)
if (!scripts_dir)
return NULL;

- for_each_lang(scripts_dir, lang_dirent, lang_next) {
+ for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {
snprintf(lang_path, MAXPATHLEN, "%s/%s/bin", scripts_path,
lang_dirent.d_name);
lang_dir = opendir(lang_path);
if (!lang_dir)
continue;

- for_each_script(lang_dir, script_dirent, script_next) {
+ for_each_script(lang_path, lang_dir, script_dirent, script_next) {
__script_root = strdup(script_dirent.d_name);
str = ends_with(__script_root, suffix);
if (str) {