Em Mon, Nov 04, 2013 at 12:06:11PM +0100, Jiri Olsa escreveu:
> Moving sysfs code into generic fs object and preparing
> it to carry procfs support.
Cool, but these are not 'perf' specific at all, so, please see below
some requests/suggestions:
>
> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> index bc7cfa1..f5d3251 100644
> --- a/tools/perf/Makefile.perf
> +++ b/tools/perf/Makefile.perf
> @@ -242,7 +242,7 @@ LIB_H += util/cache.h
> LIB_H += util/callchain.h
> LIB_H += util/build-id.h
> LIB_H += util/debug.h
> -LIB_H += util/sysfs.h
> +LIB_H += util/fs.h
Perfect naming, 'fs'
> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index beb8cf9..4af5a23 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -1,5 +1,5 @@
> #include "util.h"
> -#include "sysfs.h"
> +#include "fs.h"
> #include "../perf.h"
> #include "cpumap.h"
> #include <assert.h>
> diff --git a/tools/perf/util/fs.c b/tools/perf/util/fs.c
> new file mode 100644
> index 0000000..346669a
> --- /dev/null
> +++ b/tools/perf/util/fs.c
> @@ -0,0 +1,107 @@
> +
> +/* TODO merge/factor into tools/lib/lk/debugfs.c */
> +
> +#include "util.h"
> +#include "util/fs.h"
> +
> +static const char * const sysfs_known_mountpoints[] = {
> + "/sys",
> + 0,
> +};
> +
> +struct perf_fs {
Ditch the 'perf', make it plain 'struct fs'.
> + const char *name;
> + const char * const *mounts;
> + char path[PATH_MAX + 1];
> + bool found;
> + long magic;
> +};
> +
> +enum {
> + FS_SYSFS = 0,
FS__SYSFS
> +};
> +
> +static struct perf_fs fss[] = {
Funny name, perhaps fs__entries instead? :-)
And here we have it static, at some point we could introduce a
'fs__register', that would be the counterpart of 'register_filesystem'
in the kernel sources.
> + [FS_SYSFS] = {
> + .name = "sysfs",
> + .mounts = sysfs_known_mountpoints,
> + .magic = SYSFS_MAGIC,
> + },
> +};
> +
> +static bool read_mounts(struct perf_fs *fs)
> +{
> + bool found = false;
> + char type[100];
> + FILE *fp;
> +
> + fp = fopen("/proc/mounts", "r");
> + if (fp == NULL)
> + return NULL;
> +
> + while (!found &&
> + fscanf(fp, "%*s %" STR(PATH_MAX) "s %99s %*s %*d %*d\n",
> + fs->path, type) == 2) {
> +
> + if (strcmp(type, fs->name) == 0)
> + found = true;
> + }
> +
> + fclose(fp);
> + return fs->found = found;
> +}
This is not a per instance method, I to traverse /proc/mounts once,
checking all entries in 'fs__entries' marking the ones that are present,
i.e. fs__entries would be a list/tree of 'struct fs'.
> +static int valid_mount(const char *fs, long magic)
> +{
> + struct statfs st_fs;
> +
> + if (statfs(fs, &st_fs) < 0)
> + return -ENOENT;
> + else if (st_fs.f_type != magic)
> + return -ENOENT;
> +
> + return 0;
> +}
Now this is getting me confused, so we will have to traverse
/proc/mounts looking for that .name to be what we expect in entries and
afterwards we do a second step, checking if the magic number is the one
expected? Can't we do both verifications in just one place?
I know you haven't written this code, is just generalizing, but I got
confused so had to comment on it :-\
Perhaps since you're just making it useful for more filesystems just
please address the 'perf_fs' naming suggestions and we can deal with
these other issues later?
- Arnaldo
> +static bool check_mounts(struct perf_fs *fs)
> +{
> + const char * const *ptr;
> +
> + ptr = fs->mounts;
> + while (*ptr) {
> + if (valid_mount(*ptr, fs->magic) == 0) {
> + fs->found = true;
> + strcpy(fs->path, *ptr);
> + return true;
> + }
> + ptr++;
> + }
> +
> + return false;
> +}
> +
> +static const char *get_mountpoint(struct perf_fs *fs)
> +{
> + if (check_mounts(fs))
> + return fs->path;
> +
> + return read_mounts(fs) ? fs->path : NULL;
> +}
> +
> +static const char *find_mountpoint(int idx)
> +{
> + struct perf_fs *fs = &fss[idx];
> +
> + if (fs->found)
> + return (const char *) fs->path;
> +
> + return get_mountpoint(fs);
> +}
> +
> +#define FIND_MOUNTPOINT(name, idx) \
> +const char *name##_find_mountpoint(void) \
> +{ \
> + return find_mountpoint(idx); \
> +}
> +
> +FIND_MOUNTPOINT(sysfs, FS_SYSFS);
> diff --git a/tools/perf/util/fs.h b/tools/perf/util/fs.h
> new file mode 100644
> index 0000000..082edbd
> --- /dev/null
> +++ b/tools/perf/util/fs.h
> @@ -0,0 +1,6 @@
> +#ifndef __PERF_FS
> +#define __PERF_FS
> +
> +const char *sysfs_find_mountpoint(void);
> +
> +#endif /* __PERF_FS */
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 64362fe..45b42df 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -4,7 +4,7 @@
> #include <unistd.h>
> #include <stdio.h>
> #include <dirent.h>
> -#include "sysfs.h"
> +#include "fs.h"
> #include "util.h"
> #include "pmu.h"
> #include "parse-events.h"
> diff --git a/tools/perf/util/python-ext-sources
> b/tools/perf/util/python-ext-sources
> index f75ae1b..239036f 100644
> --- a/tools/perf/util/python-ext-sources
> +++ b/tools/perf/util/python-ext-sources
> @@ -17,5 +17,5 @@ util/xyarray.c
> util/cgroup.c
> util/rblist.c
> util/strlist.c
> -util/sysfs.c
> +util/fs.c
> ../../lib/rbtree.c
> diff --git a/tools/perf/util/sysfs.c b/tools/perf/util/sysfs.c
> deleted file mode 100644
> index f71e9ea..0000000
> --- a/tools/perf/util/sysfs.c
> +++ /dev/null
> @@ -1,60 +0,0 @@
> -
> -#include "util.h"
> -#include "sysfs.h"
> -
> -static const char * const sysfs_known_mountpoints[] = {
> - "/sys",
> - 0,
> -};
> -
> -static int sysfs_found;
> -char sysfs_mountpoint[PATH_MAX + 1];
> -
> -static int sysfs_valid_mountpoint(const char *sysfs)
> -{
> - struct statfs st_fs;
> -
> - if (statfs(sysfs, &st_fs) < 0)
> - return -ENOENT;
> - else if (st_fs.f_type != (long) SYSFS_MAGIC)
> - return -ENOENT;
> -
> - return 0;
> -}
> -
> -const char *sysfs_find_mountpoint(void)
> -{
> - const char * const *ptr;
> - char type[100];
> - FILE *fp;
> -
> - if (sysfs_found)
> - return (const char *) sysfs_mountpoint;
> -
> - ptr = sysfs_known_mountpoints;
> - while (*ptr) {
> - if (sysfs_valid_mountpoint(*ptr) == 0) {
> - sysfs_found = 1;
> - strcpy(sysfs_mountpoint, *ptr);
> - return sysfs_mountpoint;
> - }
> - ptr++;
> - }
> -
> - /* give up and parse /proc/mounts */
> - fp = fopen("/proc/mounts", "r");
> - if (fp == NULL)
> - return NULL;
> -
> - while (!sysfs_found &&
> - fscanf(fp, "%*s %" STR(PATH_MAX) "s %99s %*s %*d %*d\n",
> - sysfs_mountpoint, type) == 2) {
> -
> - if (strcmp(type, "sysfs") == 0)
> - sysfs_found = 1;
> - }
> -
> - fclose(fp);
> -
> - return sysfs_found ? sysfs_mountpoint : NULL;
> -}
> diff --git a/tools/perf/util/sysfs.h b/tools/perf/util/sysfs.h
> deleted file mode 100644
> index a813b72..0000000
> --- a/tools/perf/util/sysfs.h
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -#ifndef __SYSFS_H__
> -#define __SYSFS_H__
> -
> -const char *sysfs_find_mountpoint(void);
> -
> -#endif /* __DEBUGFS_H__ */
> --
> 1.7.11.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
