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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131105130729.GD1071@krava.brq.redhat.com>
Date:	Tue, 5 Nov 2013 14:07:29 +0100
From:	Jiri Olsa <jolsa@...hat.com>
To:	Arnaldo Carvalho de Melo <acme@...hat.com>
Cc:	linux-kernel@...r.kernel.org,
	Adrian Hunter <adrian.hunter@...el.com>,
	Corey Ashford <cjashfor@...ux.vnet.ibm.com>,
	David Ahern <dsahern@...il.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...e.hu>,
	Namhyung Kim <namhyung@...nel.org>,
	Paul Mackerras <paulus@...ba.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/