Message-ID: <877ft2d64y.fsf_-_@x220.int.ebiederm.org>
Date:	Thu, 23 Apr 2015 13:57:49 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Arnd Bergmann <arnd@...db.de>, gnomes@...rguk.ukuu.org.uk,
	teg@...m.no, jkosina@...e.cz, luto@...capital.net,
	linux-kernel@...r.kernel.org, daniel@...que.org,
	dh.herrmann@...il.com, tixxdz@...ndz.org
Subject: Kdbus needs meaningful review (was: Re: [GIT PULL] kdbus for 4.1-rc1)

Greg Kroah-Hartman <gregkh@...uxfoundation.org> writes:

> On Mon, Apr 13, 2015 at 09:03:50PM +0200, Greg Kroah-Hartman wrote:
>> The following changes since commit 9eccca0843205f87c00404b663188b88eb248051:
>> 
>>   Linux 4.0-rc3 (2015-03-08 16:09:09 -0700)
>> 
>> are available in the git repository at:
>> 
>>   git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/ tags/kdbus-4.1-rc1
>> 
>> for you to fetch changes up to 9fb9cd0f4434a23487b6ef3237e733afae90e336:
>> 
>>   kdbus: avoid the use of struct timespec (2015-04-10 14:34:53 +0200)
>> 
>> ----------------------------------------------------------------
>
> Given this has been a crazy email thread, let's try to figure out what
> the status is here.
>
> Al Viro pointed out some odd locking (r/w lock only used in write mode),
> and asked for some more documentation / description of the object model
> used here.  David provided that, and will send a minor fix for the rw
> lock, so I think that issue is now resolved.  David has created a few
> other minor changes based on Al's review that I will forward on later.

As I recall Al could not even trace through all of the locking, and that
prevented him looking at any of the bigger issues.  That does not
qualify as fixed with a little patch or two and everything thing is
fixed.

Perhaps with the minor patch or two Al could potentially actually review
the code.

> Andy's concerns about the capability stuff has been hashed out in
> multiple threads here.  The kernel code isn't buggy as-designed or
> implemented from what we can all tell, it's just that the new
> functionality isn't liked by everyone, which is totally fair, but not a
> reason to declare that the function isn't useful.

There are in fact implementation bugs and you asserion that there are
none is the largest reason this code should not be merged.  You are
turning a blind eye to problems.

> Alan, and others, want a tiny, generic, multi-cast IPC method that also
> works across networks.  They feel that this is something that D-Bus
> might be able to use in the future in userspace to build on top of. 
> Lots of people have said they want something like this for years, but
> that doesn't address the issue here with kdbus, which is a very specific
> solution for a very common and wide-spread usage model that Linux
> userspace relies on today.  I too would love to see such an IPC be
> created, and two years ago thought it would be possible to achieve
> here.  But over time, and in working with the D-Bus model and
> requirements, it just didn't happen here.  Given that no one has ever
> been able to accomplish such a thing in the past means that it's either
> impossible to do, or that no one really wants such a thing bad enough to
> actually do the work :)

What is the rush?

> Did I miss anything else here?  Are there any technical reasons I'm
> forgetting about for why this can't be pulled in as-is for this merge
> window?

****The code has not been meaningfully or properly reviewed.****


Greg you are pushing entirely too hard for this code to get it.  When
someone pushes as hard as you are doing, inevitiable problems get
through.

Greg it is my professional opinion that the code smells.  There are all
sort of missteps and oversights that indicate that almost certainly that
something important has been overlooked.

I do not believe this code is yet up to the standards we want for core
kernel code.

This code has astonishingly complex interactions with all kinds of other
kernel subsystems and concerns.  As a community we should understand
them and accept them before letting them in.


The only way I have seen anything make meaninful progress with those
kinds of interactions is for the pieces to be teased apart.  And then
the code incrementally added to with all of the right people being
pulled into the discussion.

I suspect removing all of the extensions to the capabilities of dbus-1
would be a good start for getting a piece of code that could be
meaningfully reviewed.  Then the controversial bits can be addressed on
their own.  As it is there is too much for to properly address any one
issue.


Greg this process has fundamentally not given people time to understand
the code, the interactions or the complexities.  The discussions show
that.


Further by refusing to tease apart the pieces.  By refusing to allow
other people time to understand this code.  By refusing to give an inch
and admit anyone else has a valid point real problems, and real issues
can not be revealed and fixed.

With such a pig headed direction I do not believe that kdbus is in any
sense ready for merging.


Eric

p.s. One of the issues of smell that I have been talking about is I see
in kdbus patterns of code construction that have caused real world
performance problems, and real world security issues.  And those issues
get ignored when brought up.

p.p.s Not that this complaint is not in any sense new you have been
ignoring people who try to bring up meaningful issues for a long time.
The fact that when people bring up uncomfortable points about the kdbus
code they get routingely blown off certainly contributes to the lack of
meaningful review as it is not rewarding to work with someone who does
not listen to criticism.  At this point the strongest possible language
and the strongest possible push back are being used because everything
else is routinely swept under the rug.


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

Powered by blists - more mailing lists