Steven Rostedt wrote:
On Thu, 23 Apr 2020 17:41:52 +0200
Christophe Leroy <christophe.le...@c-s.fr> wrote:
> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> index 024f7aad1952..046485bb0a52 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -139,52 +139,67 @@ void arch_remove_optimized_kprobe(struct
optimized_kprobe *op)
> }
> }
>
> +#define PATCH_INSN(addr, instr) \
> +do {
\
> + int rc = patch_instruction((unsigned int *)(addr), instr); \
> + if (rc) { \
> + pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n", \
> + __func__, __LINE__, \
> + (void *)(addr), (void *)(addr), rc); \
> + return rc; \
> + } \
> +} while (0)
> +
I hate this kind of macro which hides the "return".
What about keeping the return action in the caller ?
Otherwise, what about implementing something based on the use of goto,
on the same model as unsafe_put_user() for instance ?
Thanks for the review.
I noticed this as a warning from checkpatch.pl, but this looked compact
and correct for use in the two following functions. You'll notice that I
added it just before the two functions this is used in.
I suppose 'goto err' is usable too, but the ftrace code (patch 2) will
end up with more changes. I'm also struggling to see how a 'goto' is
less offensive. I think Steve's suggestion below would be the better way
to go, to make things explicit.
#define PATCH_INSN(addr, instr) \
({
int rc = patch_instruction((unsigned int *)(addr), instr); \
if (rc) \
pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n",
\
__func__, __LINE__, \
(void *)(addr), (void *)(addr), rc); \
rc; \
})
Then you can just do:
ret = PATCH_INSN(...);
if (ret)
return ret;
in the code.
That's really nice. However, in this case, I guess I can simply use an
inline function? The primary reason I used the macro was for including a
'return' statement in it.
Thanks for the review!
- Naveen
