Expand the if condition into cascaded ifs to make code
readable.
Also use sizeof(union mips_instruction) instead of
sizeof(mips_instruction) to match the code context.
Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
arch/mips/kernel/kprobes.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
index dc39f5b3fb83..96139adefad2 100644
--- a/arch/mips/kernel/kprobes.c
+++ b/arch/mips/kernel/kprobes.c
@@ -89,12 +89,12 @@ int arch_prepare_kprobe(struct kprobe *p)
goto out;
}
- if (copy_from_kernel_nofault(&prev_insn, p->addr - 1,
- sizeof(mips_instruction)) == 0 &&
- insn_has_delayslot(prev_insn)) {
- pr_notice("Kprobes for branch delayslot are not supported\n");
- ret = -EINVAL;
- goto out;
+ if (!copy_from_kernel_nofault(&prev_insn, p->addr - 1, sizeof(union mips_instruction))) {
+ if (insn_has_delayslot(prev_insn)) {
+ pr_notice("Kprobes for branch delayslot are not supported\n");
+ ret = -EINVAL;
+ goto out;
+ }
}
if (__insn_is_compact_branch(insn)) {
--
2.46.0On Sun, 8 Sep 2024, Jiaxun Yang wrote:
> Expand the if condition into cascaded ifs to make code
> readable.
Apart from broken formatting what's making original code unreadable?
> Also use sizeof(union mips_instruction) instead of
> sizeof(mips_instruction) to match the code context.
That has to be a separate change.
> diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
> index dc39f5b3fb83..96139adefad2 100644
> --- a/arch/mips/kernel/kprobes.c
> +++ b/arch/mips/kernel/kprobes.c
> @@ -89,12 +89,12 @@ int arch_prepare_kprobe(struct kprobe *p)
> goto out;
> }
>
> - if (copy_from_kernel_nofault(&prev_insn, p->addr - 1,
> - sizeof(mips_instruction)) == 0 &&
> - insn_has_delayslot(prev_insn)) {
> - pr_notice("Kprobes for branch delayslot are not supported\n");
> - ret = -EINVAL;
> - goto out;
> + if (!copy_from_kernel_nofault(&prev_insn, p->addr - 1, sizeof(union mips_instruction))) {
Overlong line.
> + if (insn_has_delayslot(prev_insn)) {
> + pr_notice("Kprobes for branch delayslot are not supported\n");
This now overruns 80 columns making code *less* readable.
Maciej
在2024年9月9日九月 下午11:02，Maciej W. Rozycki写道：
> On Sun, 8 Sep 2024, Jiaxun Yang wrote:
>
>> Expand the if condition into cascaded ifs to make code
>> readable.
>
> Apart from broken formatting what's making original code unreadable?
For me it's confusing because wired layout, cascaded ifs are clearly
easier to format and has clear intention.
>
>> Also use sizeof(union mips_instruction) instead of
>> sizeof(mips_instruction) to match the code context.
>
> That has to be a separate change.
Given that it's a tiny style change as well, it makes sense to combine
into same patch.
>
>> diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
>> index dc39f5b3fb83..96139adefad2 100644
>> --- a/arch/mips/kernel/kprobes.c
>> +++ b/arch/mips/kernel/kprobes.c
>> @@ -89,12 +89,12 @@ int arch_prepare_kprobe(struct kprobe *p)
>> goto out;
>> }
>>
>> - if (copy_from_kernel_nofault(&prev_insn, p->addr - 1,
>> - sizeof(mips_instruction)) == 0 &&
>> - insn_has_delayslot(prev_insn)) {
>> - pr_notice("Kprobes for branch delayslot are not supported\n");
>> - ret = -EINVAL;
>> - goto out;
>> + if (!copy_from_kernel_nofault(&prev_insn, p->addr - 1, sizeof(union mips_instruction))) {
>
> Overlong line.
Nowadays, check-patch.pl is happy with 100 column line.
I used 100 column line in many subsystems and never receive any complaint.
>
>> + if (insn_has_delayslot(prev_insn)) {
>> + pr_notice("Kprobes for branch delayslot are not supported\n");
>
> This now overruns 80 columns making code *less* readable.
I don't really agree, we are not in VGA display era any more, see Linus's
arguments on removal of 80 columns [1] and why long line are more readable [2].
[1]: https://lore.kernel.org/lkml/CAHk-=wj3iGQqjpvc+gf6+C29Jo4COj6OQQFzdY0h5qvYKTdCow@mail.gmail.com/
[2]: https://lore.kernel.org/lkml/CAHk-=wjR0H3+2ba0UUWwoYzYBH0GX9yTf5dj2MZyo0xvyzvJnA@mail.gmail.com/
Thanks
>
> Maciej
--
- Jiaxun
On Tue, 10 Sep 2024, Jiaxun Yang wrote:
> >> + if (insn_has_delayslot(prev_insn)) {
> >> + pr_notice("Kprobes for branch delayslot are not supported\n");
> >
> > This now overruns 80 columns making code *less* readable.
>
> I don't really agree, we are not in VGA display era any more, see Linus's
> arguments on removal of 80 columns [1] and why long line are more readable [2].
Human perception hasn't changed though and just that you can squeeze a
vast number of characters in a line it doesn't mean you can parse them
comfortably with eyes. Printed books have a common line length limit too,
established with centuries of experience. Some projects such as GDB
prefer a lower soft limit of 74 characters even (with 80 being the hard
one), exactly for this reason[1].
NB even back in 1990s there was an option to use 132-character lines with
SVGA hardware in text mode, but hardly anybody used that because, well, it
didn't make text any more readable. Rather one became lost right away.
Last but not least Documentation/process/coding-style.rst still mandates
80-column formatting.
References:
[1] <https://sourceware.org/ml/gdb-patches/2014-01/msg00216.html>
Maciej
On Tue, Sep 10, 2024 at 10:43:23AM +0100, Jiaxun Yang wrote: > > > 在2024年9月9日九月 下午11:02，Maciej W. Rozycki写道： > > On Sun, 8 Sep 2024, Jiaxun Yang wrote: > > > >> Expand the if condition into cascaded ifs to make code > >> readable. > > > > Apart from broken formatting what's making original code unreadable? > > For me it's confusing because wired layout, cascaded ifs are clearly > easier to format and has clear intention. I prefer the original version, it's just to statements combined with &&, which isn't scary at all. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ]
© 2016 - 2024 Red Hat, Inc.