lkml.org 
[lkml]   [2011]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v7 3.2-rc2 4/30] uprobes: Define hooks for mmap/munmap.
    From
    Date
    On Tue, 2011-11-29 at 21:52 +0530, Srikar Dronamraju wrote:
    > The rules that I am using are:
    >
    > mmap_uprobe() increments the count if
    > - it successfully adds a breakpoint.
    > - it not add a breakpoint, but sees that there is a underlying
    > breakpoint (via a read_opcode call).
    >
    > munmap_uprobe() decrements the count if
    > - it sees a underlying breakpoint, (via a read_opcode call)
    > - Subsequent unregister_uprobe wouldnt find the breakpoint
    > unless a mmap_uprobe kicks in, since the old vma would be
    > dropped just after munmap_uprobe.
    >
    > register_uprobe increments the count if:
    > - it successfully adds a breakpoint.
    >
    > unregister_uprobe decrements the count if:
    > - it sees a underlying breakpoint and removes successfully.
    > (via a read_opcode call)
    > - Subsequent munmap_uprobe wouldnt find the breakpoint
    > since there is no underlying breakpoint after the
    > breakpoint removal.

    The problem I'm having is that such stuff isn't included in the patch
    set.

    We've got both comments in the C language and Changelog in our patch
    system, yet you consistently fail to use either to convey useful
    information on non-trivial bits like this.

    This leaves the reviewer wondering if you've actually considered stuff
    properly, then me actually finding bugs in there does of course
    undermine that even further.

    What I really would like is for this patch set not to have such subtle
    stuff at all, esp. at first. Once its in and its been used a bit we can
    start optimizing and add subtle crap like this.




    \
     
     \ /
      Last update: 2011-11-30 13:29    [W:2.965 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site