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

[Gleb Natapov <gleb@redhat.com>]

On Tue, Jan 24, 2012 at 12:23:07PM +0100, Kevin Wolf wrote:
> 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 {
>                 ....
>         }
> "
> 
I saw that, but here it talks about something different: if closing
brace should be the only thing on a line or not. It does not specify how
much statements ... represents. Example is chosen to show the point
paragraph before it tries to make and for that you need braces.

> > 
> >>> 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.
> 

--
			Gleb.