Re: [PATCH v2 1/4] trace-cmd: Add parse error checking target

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 2011-07-25 at 11:39 -0700, Vaibhav Nagarnaik wrote:
> Add another target 'check-events' which parses all the event formats and
> returns whether there are any issues with the print format strings.
> 
> With an error in the format, the return value is 22 (EINVAL) and for
> success, it is 0.

Could you write up a man page for this too?


> Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@google.com>
> ---
> Changelog v2-v1:
> * Pass any parsing failures in pevent structure
> 
>  parse-events.h |    2 ++
>  trace-cmd.c    |   25 +++++++++++++++++++++++++
>  trace-usage.c  |    5 +++++
>  trace-util.c   |   36 ++++++++++++++++++++++++++----------
>  4 files changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/parse-events.h b/parse-events.h
> index c32d715..f5cab15 100644
> --- a/parse-events.h
> +++ b/parse-events.h
> @@ -402,6 +402,8 @@ struct pevent {
>  	struct event_handler *handlers;
>  	struct pevent_function_handler *func_handlers;
>  
> +	int parsing_failures;
> +
>  	/* cache */
>  	struct event_format *last_event;
>  };
> diff --git a/trace-cmd.c b/trace-cmd.c
> index bff5bbf..5cfd97f 100644
> --- a/trace-cmd.c
> +++ b/trace-cmd.c
> @@ -158,6 +158,31 @@ int main (int argc, char **argv)
>  	} else if (strcmp(argv[1], "stack") == 0) {
>  		trace_stack(argc, argv);
>  		exit(0);
> +	} else if (strcmp(argv[1], "check-events") == 0) {
> +		char *tracing;
> +		int ret;
> +		struct pevent *pevent = NULL;
> +
> +		tracing = tracecmd_find_tracing_dir();
> +
> +		if (!tracing) {
> +			printf("Can not find or mount tracing directory!\n"
> +				"Either tracing is not configured for this "
> +				"kernel\n"
> +				"or you do not have the proper permissions to "
> +				"mount the directory");
> +			exit(EINVAL);
> +		}
> +
> +		ret = 0;
> +		pevent = tracecmd_local_events(tracing);
> +		if (!pevent)
> +			exit(EINVAL);
> +		if (pevent->parsing_failures)
> +			ret = EINVAL;

Hmm, what about doing:

	if (!pevent || pevent->parsing_failures)
		ret = EINVAL;



> +		pevent_free(pevent);

And allow pevent_free() to take a NULL pointer?

Hmm, I'll just apply this patch and then make the update. But could you
still send a man page patch?

Thanks!

-- Steve

> +		exit(ret);
> +
>  	} else if (strcmp(argv[1], "record") == 0 ||
>  		   strcmp(argv[1], "start") == 0 ||
>  		   strcmp(argv[1], "extract") == 0 ||
> diff --git a/trace-usage.c b/trace-usage.c
> index 39c8fc1..58ef167 100644
> --- a/trace-usage.c
> +++ b/trace-usage.c
> @@ -150,6 +150,11 @@ static struct usage_help usage_help[] = {
>  		"          --reset  reset the maximum stack found\n"
>  	},
>  	{
> +		"check-events",
> +		"parse trace event formats",
> +		" %s check-format\n"
> +	},
> +	{
>  		NULL, NULL, NULL
>  	}
>  };
> diff --git a/trace-util.c b/trace-util.c
> index 674f37e..01894f8 100644
> --- a/trace-util.c
> +++ b/trace-util.c
> @@ -621,22 +621,22 @@ static int read_file(const char *file, char **buffer)
>  	return len;
>  }
>  
> -static void load_events(struct pevent *pevent, const char *system,
> +static int load_events(struct pevent *pevent, const char *system,
>  			const char *sys_dir)
>  {
>  	struct dirent *dent;
>  	struct stat st;
>  	DIR *dir;
>  	int len = 0;
> -	int ret;
> +	int ret = 0, failure = 0;
>  
>  	ret = stat(sys_dir, &st);
>  	if (ret < 0 || !S_ISDIR(st.st_mode))
> -		return;
> +		return EINVAL;
>  
>  	dir = opendir(sys_dir);
>  	if (!dir)
> -		return;
> +		return errno;
>  
>  	while ((dent = readdir(dir))) {
>  		const char *name = dent->d_name;
> @@ -662,15 +662,18 @@ static void load_events(struct pevent *pevent, const char *system,
>  		if (len < 0)
>  			goto free_format;
>  
> -		pevent_parse_event(pevent, buf, len, system);
> +		ret = pevent_parse_event(pevent, buf, len, system);
>  		free(buf);
>   free_format:
>  		free(format);
>   free_event:
>  		free(event);
> +		if (ret)
> +			failure = ret;
>  	}
>  
>  	closedir(dir);
> +	return failure;
>  }
>  
>  static int read_header(struct pevent *pevent, const char *events_dir)
> @@ -715,7 +718,7 @@ struct pevent *tracecmd_local_events(const char *tracing_dir)
>  	char *events_dir;
>  	struct stat st;
>  	DIR *dir;
> -	int ret;
> +	int ret, failure = 0;
>  
>  	if (!tracing_dir)
>  		return NULL;
> @@ -725,21 +728,28 @@ struct pevent *tracecmd_local_events(const char *tracing_dir)
>  		return NULL;
>  
>  	ret = stat(events_dir, &st);
> -	if (ret < 0 || !S_ISDIR(st.st_mode))
> +	if (ret < 0 || !S_ISDIR(st.st_mode)) {
> +		failure = 1;
>  		goto out_free;
> +	}
>  
>  	dir = opendir(events_dir);
> -	if (!dir)
> +	if (!dir) {
> +		failure = 1;
>  		goto out_free;
> +	}
>  
>  	pevent = pevent_alloc();
> -	if (!pevent)
> +	if (!pevent) {
> +		failure = 1;
>  		goto out_free;
> +	}
>  
>  	ret = read_header(pevent, events_dir);
>  	if (ret < 0) {
>  		pevent_free(pevent);
>  		pevent = NULL;
> +		failure = 1;
>  		goto out_free;
>  	}
>  
> @@ -758,9 +768,12 @@ struct pevent *tracecmd_local_events(const char *tracing_dir)
>  			continue;
>  		}
>  
> -		load_events(pevent, name, sys);
> +		ret = load_events(pevent, name, sys);
>  
>  		free(sys);
> +
> +		if (ret)
> +			failure = 1;
>  	}
>  
>  	closedir(dir);
> @@ -768,6 +781,9 @@ struct pevent *tracecmd_local_events(const char *tracing_dir)
>   out_free:
>  	free(events_dir);
>  
> +	if (pevent)
> +		pevent->parsing_failures = failure;
> +
>  	return pevent;
>  }
>