lkml.org 
[lkml]   [2011]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 1/4] trace-cmd: Add parse error checking target
    On Mon, Jul 25, 2011 at 6:32 AM, Steven Rostedt <rostedt@goodmis.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@google.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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-07-25 20:09    [W:0.060 / U:0.376 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site