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

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: <CAL26m8Lex-snZFTt-dBDgx5WeQ8StYSr=50oyZ=J1YGf0+JUGQ@mail.gmail.com>
Date:	Mon, 25 Jul 2011 11:06:00 -0700
From:	Vaibhav Nagarnaik <vnagarnaik@...gle.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Michael Rubin <mrubin@...gle.com>,
	David Sharp <dhsharp@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] trace-cmd: Add parse error checking target

On Mon, Jul 25, 2011 at 6:32 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
> 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@...gle.com>
>> ---
>>  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.

I agree it is ugly, but I wanted to preserve the legacy behavior where
even with parsing failures, tracecmd_local_events() returns a filled in
parsed events. This is the easiest way to return a filled in pevent and
indicate whether there were *any* parsing failures.

Now that I think about it, I can add the boolean in the returned pevent
structure to have the same effect and keep the same constructor
signature. I will send the updated patch in a moment.

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

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