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

[Jiri Olsa <jolsa@redhat.com>]

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