Re: [PATCH] vt: add an event interface

[Ingo Molnar]

* Alan Cox <alan@xxxxxxxxxxxxxxxxxxx> wrote:

> > > Its called engineering and good practice. The old code was 
> > > correct. The paste of it is therefore most likely to be 
> > > correct. Furthermore patches shouldn't mix clean up with other 
> > > changes. So doing it as one is most definitely bad practice.
> > 
> > So you cannot do such trivial cleanups safely and validate the 
> > result?
> 
> What part of not mixing clean-up with other changes didn't you 
> understand ? [...]

It's an argument you made on false premises.

The thing is, my review wasnt about old code being moved around. It 
was about new code being introduced by you:

+       if (copy_from_user(&vw.event, (void __user *)arg, sizeof(struct vt_event)))
+               return -EFAULT;
+       /* Highest supported event for now */
+       if(vw.event.event > VT_MAX_EVENT)
+               return -EINVAL;

Same pattern as in the old commit: you used differing styles just 3 
lines apart in new code and apparently didnt even notice it. So i 
dont buy your argument at all that you will do 'cleanups later' as 
your patch shows ignorance of the whole concept.

Compromising on quality only results in crap being ingrained, it 
results in procrastinating and it results in you submitting new code 
with basic problems - the very patch here i commented on.

Nor is you assertion correct that fixing basic issues in it has to 
degrade it reliability: it can be done safely. In my experience 
striving for quality in such a way improves the end result. (the 
exception is when moving around whole files or significant chunks of 
code - then we preserve the old one, then move it.)

> > I came across this patch of yours on lkml and noticed a few 
> > problems - checkpatch noticed a few more. Is the reviewing of 
> > patches and the commenting on unclean patches a 'bogus 
> > standard'? Do we have some separate standard just for you?
> 
> checkpatch is a tool, not a religion. [...]

The thing is, you didnt really answer my question whether you 
consider yourself to be accountable to the same standards of quality 
as other upstream kernel contributors?

Obviously checkpatch is not a religion - in fact in my reply i gave 
a specific example where checkpatch would complain but that 
complaint is wrong. Checkpatch is a tool, and the problem is not 
that you are not making use of that tool - the problem is that you 
are producing unclean code. If you wrote clean code there would be 
no reason for anyone to complain.

So just like the religious application of checkpatch is not 
acceptable, is the religious avoidance of checkpatch not acceptable 
either ;-)

> [...] Quite frankly the way some people behave with it is a 
> disgrace and it puts people off contributing to the kernel when 
> their 500 line driver gets nothing but emails from people saying 
> "that space is wrong". At least in your case you can actually 
> program and your opinion comes from real practise and experience - 
> which is more than a lot of the self appointed codingstyle police 
> can say.

But you are not a newbie driver submitter, are you? Do you consider 
yourself exempt from accepted rules of cleanliness, for new code you 
submit?

[...]

> > Anyway ... all i did was to comment on a somewhat deficient 
> > patch that you submitted to lkml. If you dont want review 
> > feedback and if you feel embarrased and defensive by getting it 
> > then please dont send out slightly-messy signed-off patches to 
> > lkml, it's simple as that.
> 
> The message I thought was quite clear about what I was asking for. 
> The fact you complained about spaces and missed the fact it 
> couldn't even work was "enlightening" in terms of code review.

Here, in support of advancing a false argument you materially 
distort the review i gave you, which, in part, said:

| [...]
|
|   The cast to void __user * should be done in the ioctl 
|   demultiplexer vt_ioctl(), and the ioctl ugliness should not 
|   invade cleaner child functions such as vt_event_wait_ioctl().
|
| - same for vt_event_wait_ioctl() - it passes in a type damaged by
|   ioctl's limitations. Such type limitations and ioctl demuxing 
|   artifacts should be kept local to vt_ioctl().

This goes beyond 'spaces'. Type cleanliness is the first and most 
important thing when designing new interfaces - and you just added a 
new unclean interface in form of vt_event_wait_ioctl().

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/