Messages in this thread | | | Date | Tue, 25 Sep 2007 09:57:11 +0100 | From | Christoph Hellwig <> | Subject | Re: [PATCH/RFC] samples/: move kprobes sources to samples |
| |
On Tue, Sep 25, 2007 at 02:13:33PM +0530, Ananth N Mavinakayanahalli wrote: > +++ linux-2.6.23-rc7/samples/kprobes/jprobe_example.c > @@ -0,0 +1,69 @@ > +/*jprobe-example.c */
I don't think we should have this type of comment in any of the files.
> +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/fs.h> > +#include <linux/uio.h>
I don't think you'll need uio,h here.
> + * Jumper probe for do_fork. > + * Mirror principle enables access to arguments of the probed routine > + * from the probe handler. > + */ > +static const char *probed_func = "do_fork"; > +
> + /* Always end with a call to jprobe_return(). */ > + jprobe_return(); > + > + /*NOTREACHED*/ > + return 0;
I'd rather write this as:
/* Always end with a call to jprobe_return(). */ jprobe_return(); return 0; }
Also a a note not to these example but general kprobes code I've bee wondering whether jprobe_return() should just include the return. Yes, macros including a return are ugly, but in this case jprobe_return actually handles the return anyway through deep magic.
> +static struct jprobe my_jprobe = { > + .entry = jdo_fork > +}; > + > +static int __init jprobe_init(void) > +{ > + int ret; > + my_jprobe.kp.symbol_name = (char *)probed_func;
Shouldn't this be simply done in the static initialization, ala:
static struct jprobe my_jprobe = { .entry = jdo_fork, .kp = { .symbol_name = "do_fork", }, };
(same for the other examples)
> +static int handler_pre(struct kprobe *p, struct pt_regs *regs) > +{ > +#ifdef CONFIG_X86_32 > + printk("pre_handler: p->addr = 0x%p, eip = %lx, eflags = 0x%lx\n", > + p->addr, regs->eip, regs->eflags); > +#endif > +#ifdef CONFIG_X86_64 > + printk("pre_handler: p->addr = 0x%p, rip = %lx, eflags = 0x%lx\n", > + p->addr, regs->rip, regs->eflags); > +#endif > +#ifdef CONFIG_PPC > + printk("pre_handler: p->addr = 0x%p, nip = 0x%lx, msr = 0x%lx\n", > + p->addr, regs->nip, regs->msr); > +#endif
Now this is really ugly. We should really have macros for the interesting registers (instruction pointer, frame pointer, stack pointer) in kdebug.h. systemtap runtime already has them for supported architectures, any care to port them over?
(note that this is not an objection to the patch as-is, but rather a suggestion for later improvement of the whole thing)
Thanks a lot for moving this in the right place! - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |