Re: [PATCH 1/3] perf tools: Factor sysfs code into generic fs object

From: Jiri Olsa
Date: Tue Nov 05 2013 - 08:07:53 EST


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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/