Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Kevin Wolf <kwolf@redhat.com>]

Am 24.01.2012 11:52, schrieb Gleb Natapov:
> On Tue, Jan 24, 2012 at 11:38:24AM +0100, Kevin Wolf wrote:
>> Am 24.01.2012 11:17, schrieb Gleb Natapov:
>>> On Tue, Jan 24, 2012 at 11:09:09AM +0100, Kevin Wolf wrote:
>>>>>> +	} else if (reason != TASK_SWITCH_IRET) {
>>>>>> +		dpl = next_tss_desc.dpl;
>>>>>>  	}
>>>>> No need parentheses around one statement.
>>>>
>>>> Documentation/CodingStyle says:
>>>>
>>>> "This does not apply if only one branch of a conditional statement is a
>>>> single statement; in the latter case use braces in both branches:"
>>>>
>>> Then you need to put parentheses around "if (reason != TASK_SWITCH_IRET)"
>>> if you want to follow the letter of the CodingStyle :)
>>
>> Not sure what you mean. If it is 'else { if (...) { ..." then no, the
>> document isn't crazy like that.
>>
> The document says:
> 
> if (condition) {
>         do_this();
>         do_that();
> } else {
>         otherwise();
> }
> 
> So I do not see how you can interpret it otherwise.

Well, I just read more than only one paragraph. It further says:

"Note that the closing brace is empty on a line of its own, _except_ in
the cases where it is followed by a continuation of the same statement,
ie a "while" in a do-statement or an "else" in an if-statement, like
this:

        if (x == y) {
                ..
        } else if (x > y) {
                ...
        } else {
                ....
        }
"

> 
>>> But I do not see this coding stile part widely used in core kernel code:
>>> $ git grep "} else$" kernel | wc -l
>>> 122
>>>
>>> Can't think of re to check when the rule is followed :(
>>
>> Seem to be at least 77 occurences (git grep -A 2 "} else {" into a file
>> as git grep doesn't seem to do multi-line expressions and then on that
>> file "} else {\n.*\n.*}$")
>>
>> But anyway, I don't really want to discuss the right coding style here
>> and I'll apply whatever is considered right. Though if you think that
>> checkpatch.pl and Documentation/CodingStyle are both wrong, please get
>> them fixed. People might take them serious.
>>
> The code looked strange for kernel code. If checkpatch.pl complains about
> missing parentheses then they should of course stay. Our interpretation
> of CodingStyle is different.

checkpatch.pl accepts both ways, and as the counts above show they are
both used in core kernel code. If Avi or Marcelo want to have it changed
anyway, I'll resend.

Kevin