lkml.org 
[lkml]   [2011]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 1/4] trace-cmd: Add parse error checking target
    From
    Date
    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.


    > 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






    \
     
     \ /
      Last update: 2011-07-25 15:35    [W:0.032 / U:0.092 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site