On Tue, Sep 25, 2007 at 02:13:33PM +0530, Ananth N Mavinakayanahalli wrote:
quoted text
> +++ 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.

quoted text
> +#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.

quoted text
> + * 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";
> +

quoted text
> +	/* 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.

quoted text
> +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)

quoted text
> +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!
-
unsubscribe notice
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/