This is the mail archive of the mailing list for the systemtap project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: Tapset for Signals....


Just now I committed your signals tapset to CVS, with my changes as
well.  Some of my concerns below still need to be addressed, as well as
Frank's comments, but I figured that we should go ahead and put it in.
We can always improve on it going forward. :)


On Thursday, August 03, 2006 5:20 PM, Stone, Joshua I wrote:
> On Wednesday, August 02, 2006 9:27 PM, Manoj S Pattabhiraman wrote:
>> I have updated the signal tapset, with the functions you have asked
>> for. Since i would not be able to send it to the maillist, please
>> post this mail. 
>> Lemme know your suggestions ..
> Sorry I have not had time to look at this before.  I have reviewed
> your tapset now and I have some suggestions.  Some of the changes I
> went ahead and made myself, for which I have attached a new version. 
> This tapset can replace the process.signal_* probes when it is
> completed. 
> * Misc. formatting: I've made the indentation consistent throughout
> the file, and some other small changes.  Nothing major, I'm just
> picky.  :) 
> * Naming: this is more of a general pet-peeve that I have with some of
> the new tapsets.  There is no reason to name the probepoint exactly
> like the function it represents.  Unlike kernel function-naming, we
> have the flexibility to express hierarchy in the names.  To take a
> specific example, you have an alias named "signal.sendsig".  The
> trailing "sig" is redundant -- this would be better named
> "signal.send". 
> * signal.send: I did a lot of digging for this to try to capture all
> of the paths where signals can be sent.  The attached
> "send_signal.pdf" shows the callgraph as I found it.  Those 4 marked
> in blue are the points that I chose to probe for
> "process.send_signal".  The advantage to probing them separately is
> that I can expose a 'shared' variable that indicates whether the
> signal is going to a specific thread or the entire thread group.  The
> functions marked in red are those that can generate STOP/KILL
> signals, which I haven't found a convenient way to probe (perhaps
> static markers?). 
> * handle_signal: I have seen this function sometimes inlined, so this
> needs to be conditional.
> * signal.ignored: Your comment correctly states that this is really a
> _check_ whether a signal is ignored, but the name is misleading.  I
> would suggest something like "signal.check_ignored".
> * handle_stop_signal: Your comment says "fires when a stop signal is
> sent", but this is incorrect.  This function is called to _check_
> whether this signal is a stop/cont, and if so take special action.
> Thus, this will get called for almost every signal, many of which will
> not be stop signals.
> * syscall duplication: some of your probes are duplicating points from
> the syscall tapset (signal.syskill, etc.).  Do we really need these? 
> If you really want these, it would be preferable to make use of the
> syscall tapset, e.g.:
> 	probe signal.syskill = syscall.kill
> * signal.send_sig_queue: This is not needed with the change I made to
> signal.send, correct?  Consider removing if this is redundant.
> * argstr:  These arg-strings are very specific to a format that _you_
> would like.  If you look at the syscall tapset, you'll notice that the
> arg-strings are very generic, simply a comma-delimited list of the
> function arguments.  I'm not sure that what you've provided is as
> useful...
> My suggested changes are attached.  Please review my comments and
> changes, and feel free to rebut any of the above.
> Thanks,
> Josh

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]