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

[Steven Rostedt]

On Fri, 2011-07-15 at 20:00 -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.
> 
> Signed-off-by: Vaibhav Nagarnaik <vnagarnaik@xxxxxxxxxx>
> ---
>  trace-capture.c |    2 +-
>  trace-cmd.c     |   22 ++++++++++++++++++++++
>  trace-cmd.h     |    2 +-
>  trace-usage.c   |    5 +++++
>  trace-util.c    |   48 ++++++++++++++++++++++++++++++++----------------
>  5 files changed, 61 insertions(+), 18 deletions(-)
> 
> diff --git a/trace-capture.c b/trace-capture.c
> index 61ff165..5708945 100644
> --- a/trace-capture.c
> +++ b/trace-capture.c
> @@ -1295,7 +1295,7 @@ static void tracing_dialog(struct shark_info *info, const char *tracing)
>  	/* Send parse warnings to status display */
>  	trace_dialog_register_alt_warning(vpr_stat);
>  
> -	pevent = tracecmd_local_events(tracing);
> +	tracecmd_local_events(tracing, &pevent);

Ug, please no. I don't see any good reason to move the creation of a
pevent into a pointer than just return it. If you require a different
return code, or (a even better reason) that this may be called without
needing to create a pevent at all, then I can understand this. But
creating an object (sturcture) by passing its address is an anomaly of C
and I like to avoid when possible. Passing an address of a atom value
(int, long) or even maybe a string that is allocated is one thing. But
doing it with a constructor function is just plain ugly.


>  	trace_dialog_register_alt_warning(NULL);
>  
>  	cap.pevent = pevent;
> diff --git a/trace-cmd.c b/trace-cmd.c
> index bff5bbf..a2b6b91 100644
> --- a/trace-cmd.c
> +++ b/trace-cmd.c
> @@ -158,6 +158,28 @@ 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 = tracecmd_local_events(tracing, &pevent);
> +		if (pevent)
> +			pevent_free(pevent);
> +
> +		ret ? exit(0) : exit(EINVAL);
> +

And here the code is even uglier. You just free pevent and the ret is
just a boolean!  Also, that ?: trick is even uglier.


		pevent = tracecmd_local_events(tracing);
		if (!pevent)
			exit(EINVAL);
		pevent_free(pevent);
		exit(0);

Is much more readable.

-- Steve




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