Messages in this thread |  | | Date | Tue, 5 Nov 2013 10:48:14 -0200 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 1/3] perf tools: Factor sysfs code into generic fs object |
| |
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
|  |