RE: Tapset for Signals....

["Stone, Joshua I" <joshua.i.stone@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3159 bytes --]

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

[-- Attachment #2: send_signal.pdf --]
[-- Type: application/octet-stream, Size: 6887 bytes --]