Re: [PATCH 2/2] MIPS: kprobes: Massage previous delay slot detection

[Jiaxun Yang]

在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@xxxxxxxxxxxxxx/
[2]: https://lore.kernel.org/lkml/CAHk-=wjR0H3+2ba0UUWwoYzYBH0GX9yTf5dj2MZyo0xvyzvJnA@xxxxxxxxxxxxxx/

Thanks
>
>   Maciej

-- 
- Jiaxun