Christophe Leroy wrote:
On 04/24/2020 06:26 PM, Naveen N. Rao wrote:
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.
Sure it's be more explicit, but then more lines also. 3 lines for only
one really usefull.
With goto, I would look like:
diff --git a/arch/powerpc/kernel/optprobes.c
b/arch/powerpc/kernel/optprobes.c
index 046485bb0a52..938208f824da 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -139,14 +139,14 @@ void arch_remove_optimized_kprobe(struct
optimized_kprobe *op)
}
}
-#define PATCH_INSN(addr, instr)
\
+#define PATCH_INSN(addr, instr, label)
\
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; \
+ goto label; \
} \
} while (0)
My earlier complaint was that this would still add a flow control
statement, so didn't look to immediately address your original concern.
However, I suppose introduction of an explicit label makes things a bit
better.
In addition:
<snip>
@@ -291,23 +297,8 @@ int arch_prepare_optimized_kprobe(struct
optimized_kprobe *op, struct kprobe *p)
goto error;
}
- rc = patch_instruction(buff + TMPL_CALL_HDLR_IDX, branch_op_callback);
- if (rc) {
- pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
- __func__, __LINE__,
- (void *)(buff + TMPL_CALL_HDLR_IDX), rc);
- rc = -EFAULT;
- goto error;
- }
-
- rc = patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step);
- if (rc) {
- pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
- __func__, __LINE__,
- (void *)(buff + TMPL_EMULATE_IDX), rc);
- rc = -EFAULT;
- goto error;
- }
+ PATCH_INSN(buff + TMPL_CALL_HDLR_IDX, branch_op_callback, efault);
+ PATCH_INSN(buff + TMPL_EMULATE_IDX, branch_emulate_step, efault);
I like how this variant can cover additional uses of patch_instruction()
here.
I will use this variant. Thanks for the suggestion!
- Naveen
