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] [day] [month] [year] [list]
Date:	Mon, 13 Jul 2009 09:15:31 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Jason Baron <jbaron@...hat.com>
Cc:	linux-kernel@...r.kernel.org, paulus@...ba.org,
	a.p.zijlstra@...llo.nl, rostedt@...dmis.org, fweisbec@...il.com
Subject: Re: [PATCH 2/2] perf_counter: add support to the 'perf' tool for
	tracepoints


* Jason Baron <jbaron@...hat.com> wrote:

> +static char *tracepoint_id_to_name(u64 config)
> +{
> +	static char tracepoint_name[2 * MAX_EVENT_LENGTH];
> +	DIR *sys_dir, *evt_dir;
> +	struct dirent *sys_next, *evt_next, sys_dirent, evt_dirent;
> +	struct stat st;
> +	char id_buf[4];
> +	int fd;
> +	long long id;

could this become u64?

> +	char evt_path[MAX_PATH_LENGTH];
> +
> +	if (valid_debugfs_mount())
> +		return "unkown";
> +
> +	sys_dir = opendir(default_debugfs_path);
> +	if (!sys_dir)
> +		goto cleanup;
> +	for_each_subsystem(sys_dir, sys_dirent, sys_next, evt_path, st) {
> +		evt_dir = opendir(evt_path);
> +		if (!evt_dir)
> +			goto cleanup;
> +		for_each_event(sys_dirent, evt_dir, evt_dirent, evt_next,
> +								evt_path, st) {
> +			sprintf(evt_path, "%s/%s/%s/id", default_debugfs_path,
> +				sys_dirent.d_name, evt_dirent.d_name);
> +			fd = open(evt_path, O_RDONLY);
> +			if (fd < 0)
> +				continue;
> +			if (read(fd, id_buf, sizeof(id_buf)) < 0) {
> +				close(fd);
> +				continue;
> +			}
> +			close(fd);
> +			id = atoll(id_buf);
> +			if ((u64)id == config) {

... and thus the cast here could be dropped?

> +				closedir(evt_dir);
> +				closedir(sys_dir);
> +				snprintf(tracepoint_name, 2 * MAX_EVENT_LENGTH,
> +					"%s:%s", sys_dirent.d_name,
> +					evt_dirent.d_name);
> +				return tracepoint_name;
> +			}
> +		}
> +		closedir(evt_dir);
> +	}
> +cleanup:
> +	closedir(sys_dir);
> +	return "unkown";
> +}
> +
>  static int is_cache_op_valid(u8 cache_type, u8 cache_op)
>  {
>  	if (hw_cache_stat[cache_type] & COP(cache_op))
> @@ -177,6 +259,9 @@ char *event_name(int counter)
>  			return sw_event_names[config];
>  		return "unknown-software";
>  
> +	case PERF_TYPE_TRACEPOINT:
> +		return tracepoint_id_to_name(config);
> +
>  	default:
>  		break;
>  	}
> @@ -265,6 +350,78 @@ parse_generic_hw_event(const char **str, struct perf_counter_attr *attr)
>  	return 1;
>  }
>  
> +static int parse_tracepoint_event(const char **strp,
> +				    struct perf_counter_attr *attr)
> +{
> +	const char *evt_name;
> +	char sys_name[MAX_EVENT_LENGTH];
> +	DIR *sys_dir, *evt_dir;
> +	struct dirent *sys_next, *evt_next, sys_dirent, evt_dirent;
> +	struct stat st;
> +	char id_buf[4];
> +	int fd;
> +	unsigned int sys_length, evt_length;
> +	u64 id;
> +	char evt_path[MAX_PATH_LENGTH];
> +
> +	if (valid_debugfs_mount())
> +		return 0;
> +
> +	evt_name = strchr(*strp, ':');
> +	if (evt_name) {
> +		sys_length = evt_name - *strp;
> +		if (sys_length >= MAX_EVENT_LENGTH)
> +			return 0;
> +		strncpy(sys_name, *strp, sys_length);
> +		sys_name[sys_length] = '\0';
> +		evt_name = evt_name + 1;
> +		evt_length = strlen(evt_name);
> +		if (evt_length >= MAX_EVENT_LENGTH)
> +			return 0;
> +	} else
> +		return 0;
> +
> +	sys_dir = opendir(default_debugfs_path);
> +	if (!sys_dir)
> +		goto cleanup;
> +	for_each_subsystem(sys_dir, sys_dirent, sys_next, evt_path, st) {

a minor request. Just like you added a newline after the return 0 
above, please add a newline after separate blocks of code as well - 
i.e after the 'goto cleanup' above. It makes the loop which begins 
stand out on its own, and makes the sys_dir initialization look 
self-sufficient as well. (which it is)

> +		if (strcmp(sys_dirent.d_name, sys_name) == 0) {
> +			evt_dir = opendir(evt_path);
> +			if (!evt_dir)
> +				goto cleanup;
> +			for_each_event(sys_dirent, evt_dir, evt_dirent,
> +						       evt_next, evt_path, st) {
> +				if (strcmp(evt_dirent.d_name, evt_name) == 0) {
> +					sprintf(evt_path, "%s/%s/%s/id",
> +						default_debugfs_path,
> +						sys_dirent.d_name,
> +						evt_dirent.d_name);
> +					fd = open(evt_path, O_RDONLY);
> +					if (fd < 0)
> +						continue;
> +					if (read(fd, id_buf,
> +						 sizeof(id_buf)) < 0) {
> +						close(fd);
> +						continue;
> +					}
> +					close(fd);
> +					id = atoll(id_buf);
> +					attr->config = id;
> +					attr->type = PERF_TYPE_TRACEPOINT;
> +					closedir(evt_dir);
> +					closedir(sys_dir);
> +					*strp = evt_name + evt_length;
> +					return 1;
> +				}
> +			}
> +			closedir(evt_dir);
> +		}
> +	}
> +cleanup:
> +	closedir(sys_dir);
> +	return 0;
> +}

Ok, this whole feature looks nice. A few minor details need fixing: 
the two functions above tracepoint_id_to_name() and 
parse_tracepoint_event() are a bit large and look very crowded.

one trick to win an indentation level would be to turn this:

> +		if (strcmp(sys_dirent.d_name, sys_name) == 0) {
> +			evt_dir = opendir(evt_path);

into:

		if (strcmp(sys_dirent.d_name, sys_name))
			continue;

		evt_dir = opendir(evt_path);

(Also, for logic operations please write '!x' instead of 'x == 0' - 
it's easy to see only the beginning of the line and miss the 
effective negation.)

The other thing to do would be to move the inner core of the loop 
out into a helper inline function.

plus, this:

> +	if (evt_name) {
> +		sys_length = evt_name - *strp;
> +		if (sys_length >= MAX_EVENT_LENGTH)
> +			return 0;
> +		strncpy(sys_name, *strp, sys_length);
> +		sys_name[sys_length] = '\0';
> +		evt_name = evt_name + 1;
> +		evt_length = strlen(evt_name);
> +		if (evt_length >= MAX_EVENT_LENGTH)
> +			return 0;
> +	} else
> +		return 0;

should be written as:

	if (!evt_name)
		return 0;

	sys_length = evt_name - *strp;
	...

i.e. we should try to compress the visual complexity of 
code as much as possible.

Also ... all code flows return 0, regardless of error or not error. 
Is this intentional?

	Ingo
--
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/

Powered by blists - more mailing lists