Re: [PATCH v6 3/8] x86: add generic function to modify more callsusing int3 framework

[Steven Rostedt]

On Tue, 21 Jan 2014 14:50:15 +0100
Petr Mladek <pmladek@xxxxxxx> wrote:

> On Tue, 2014-01-14 at 19:33 -0500, Steven Rostedt wrote:
> > >  
> > >  	/* Patch the first byte. We do not know how to recover from an error. */
> > > -	text_poke_or_die(addr, opcode, sizeof(int3));
> > > +	text_poke_or_die(addr, opcode, sizeof(bp_int3));
> > >  
> > >  	run_sync();
> > 
> > Shouldn't we be setting the bp_int3_handler back to NULL here?
> 
> It might be cleaner but it is not really needed. "poke_int3_handler()"

Yeah, I was stating that as more of being "cleaner". I saw that the
current code handles this, but it feels like it would be more robust to
set it to NULL now.

> checks "bp_int3_handler" only when "bp_patching_in_progress" is enabled.
> The "in_progress" variable is disabled right after the above mentioned
> "run_sync()", so we are on the safe side.
> 
> Note that the original "text_poke_bp()" implementation disabled only the
> "in_progress" variable at the end as well.
> 
> 
> > > +static int add_iter_breakpoint(struct text_poke_bp_iter *iterator,
> > > +				       void *iter)
> > > +{
> > > +	void *addr;
> > > +	const void *old_opcode;
> > > +	int ret = 0;
> > > +
> > > +	/* nope if the code is not defined */
> > 
> > The above comment does not make sense.
> 
> It is here to handle the situation when "ftrace_test_record(rec,
> enable)" returns FTRACE_UPDATE_IGNORE. In this case, even the original
> implementation does not add the breakpoint.
> 
> I did not want to confuse the universal implementation with extra flags.
> Instead, I passed NULL "old_code" pointer when the patching was not
> needed for this particular address.
> 
> I agree that it might be a bit confusing. The question is whether it is
> enough to improve documentation or rather use an extra flag or so.
> 
> I am going to improve the comments unless you say otherwise.

I was confused by the comment. Please add more documentation in the
comment to explain this. You can state what it is there for (for
ftrace to ignore records) without having to add extra flags. But 10
years from now, we want developers to understand why things were done
the way they were done without having to look through email archives or
git logs.

> 
> > 
> > > +	old_opcode = iterator->get_old_opcode(iter);
> > > +	if (!old_opcode)
> > > +		return 0;
> > > +
> > > +	addr = iterator->get_addr(iter);
> > > +	ret = text_poke_check(addr, old_opcode, bp_int3_len);
> > > +
> > > +	if (likely(!ret))
> > > +		/* write the breakpoint */
> > 
> > Comment is redundant and can be removed.
> > 
> > > +		ret = text_poke(addr, &bp_int3, sizeof(bp_int3));
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int update_iter_code(struct text_poke_bp_iter *iterator,
> > > +				   void *iter)
> > > +{
> > > +	void *addr;
> > > +	const void *opcode;
> > > +
> > > +	/* nope if the code is not defined */
> > 
> > Still does not make sense :-)
> 
> It is the same reason/trick that is used in "add_iter_breakpoint()".
> NULL code pointer means that we actually do not want to patch this
> particular address.
> 

Good, please add more text to the comment to explain it better.

Thanks,

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/