Messages in this thread |  | | Date | Tue, 5 Nov 2013 14:07:29 +0100 | From | Jiri Olsa <> | Subject | Re: [PATCH 1/3] perf tools: Factor sysfs code into generic fs object |
| |
On Tue, Nov 05, 2013 at 10:48:14AM -0200, Arnaldo Carvalho de Melo wrote:
SNIP
> > +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? :-)
ok, will change all above namings :)
> > 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?
the valid_mount is called only for preconfigured (known mountpoints) paths
> > I know you haven't written this code, is just generalizing, but I got > confused so had to comment on it :-\
yep ;-) perhaps some init code could do that
> > 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?
ok, I have already changes for the 3/3 change, I'll send it together
jirka
|  |