lkml.org 
[lkml]   [2010]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 04/10][RFC] tracing: Remove per event trace registering
    From
    Date
    On Fri, 2010-04-30 at 15:06 -0400, Mathieu Desnoyers wrote:

    > > If it is possible sure, but that's the point. Where do you add the
    > > check? The typecast is in the C code that is constant for all trace
    > > events.
    >
    > You can add the call to the static inline type check directly within the
    > generated probe function, right after the local variable declarations.

    Well, one thing, the callback is not going to be the same as the
    DECLARE_TRACE() because the prototype ends with "void *data", and the
    function being called actually uses the type of that data.

    We now will have:

    DEFINE_TRACE(mytracepoint, int myarg, myarg);

    void mycallback(int myarg, struct mystuct *mydata);

    register_trace_mytracepoint_data(mycallback, mydata)

    There's no place in DEFINE_TRACE to be able to test the type of data
    that is being passed back. I could make the calling function be:

    void mycallback(int myarg, void *data)
    {
    struct mystruct *mydata = data;
    [...]

    Because the data is defined uniquely by the caller that registers a
    callback. Each function can register its own data type.

    >
    > >
    > > >
    > > > I also don't trust that these complex TRACE_EVENT() preprocessor macros
    > >
    > > Thanks for your vote of confidence.
    >
    > Please don't take this personally. As I said above, I include myself in
    > the list of people I don't trust to write entirely bug-free code. I'm
    > just saying that we should not overlook a possibility to detect more
    > bugs automatically when we have one, especially if this results in no
    > object code change.

    The point being is that this is not about buggy code, but the fact that
    the same data is being used in two places, you want to test to make sure
    it is the same. I don't see how this helps.



    >
    > >
    > > > will never ever have bugs. That's just doomed to happen one day or
    > > > another. Again, call me paranoid if you like, but I think adding this
    > > > type checking is justified.
    > >
    > > Where do you add the typecheck?? As I said before, if the TRACE_EVENT()
    > > macros are broken, then so will the typecheck, and it will not catch the
    > > errors.
    > >
    > > Sure the event macros can have bugs, but if it does then it will have
    > > bugs for all. Because it is automated. If there is a bug, it wont be
    > > because of a missed type being passed in, it would be because of one of
    > > the extra macros we have that processes the same type incorrectly.
    > >
    > > >
    > > > I am providing the type check implementation in a separate email. It
    > > > will need to be extended to support the extra data parameter you plan to
    > > > add.
    > >
    > > I saw the patch, but how does it help?
    > >
    > > I use "proto" to make the tracepoint and the callback, so I can add
    > > somewhere this "check_trace_callback_type_##name(proto)", but if the
    > > macros break somehow, that means proto changed between two references of
    > > it, but what keeps proto from breaking at both callback creation and the
    > > typecheck.
    > >
    > > Basically, you are saying that somehow the argument "proto" can change
    > > between two uses of it. I don't really see that happening, and I'm not
    > > paranoid enough to think that's an issue. Adding checks that don't
    > > really check anything, honestly I find a waste, and just more confusion
    > > in the macros.
    >
    > In the TRACE_EVENT() case, without the extra "void *data" argument,
    > it is indeed checking that the "proto" of the callback you create is
    > that same as the "proto" expected by the tracepoint call. However, given
    > that you plan on adding other parameters besides "proto", then the added
    > type-checking makes more and more sense.

    But you can not test it! That's my point.

    The first part of proto will be the same, and that's all we can test.
    But the data parameter that the DECLARE_TRACE() is going to create will
    be void *. Which means we can't test it. This is something that C lacks,
    and we could test it in C++ if we did this with templates. The only way
    to test it is at runtime with a magic number in the data field.

    This is the same as the file->private data. You can't test it at build
    time.

    Let me explain this again:

    DECLARE_TRACE(name, proto, args);

    Will call the function like:

    callback(args, data);

    The callback will be at best:

    int callback(proto, void *data);


    because the data being passed in is not defined yet. It is defined at
    the point of the registering of the callback. You can have two callbacks
    registered to the same tracepoint with two different types as the data
    field.

    So what is it that this check is testing?

    -- Steve




    \
     
     \ /
      Last update: 2010-04-30 21:51    [W:2.162 / U:0.112 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site