Re: [PATCH v2 0/5] rework access to /proc/net/rpc

[Timo Teras <timo.teras@iki.fi>]

On Tue, 09 Dec 2014 16:30:52 -0500
Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 12/09/2014 03:26 PM, David Härdeman wrote:
> > On Tue, Dec 09, 2014 at 11:08:39AM -0500, Steve Dickson wrote:
> >> On 12/09/2014 09:01 AM, David Härdeman wrote:
> >>> On 2014-12-09 09:42, Timo Teras wrote:
> >>>> On Tue, 09 Dec 2014 09:16:59 +0100
> >>>> David Härdeman <david@hardeman.nu> wrote:
> >>>>> At least the readline() function could be implemented using
> >>>>> read/write (instead of fread/fwrite) and a dynamic buffer...no?
> >>>>
> >>>> It's extra complexity. I'd rather not add it unless it's
> >>>> required. My understanding about the communication mechanism
> >>>> with kernel is that it's not required. Why have code that would
> >>>> never be used?
> >>>
> >>> I agree that it depends on your view. I tend to be very sceptical
> >>> of arbitrary limitations unless they have a very good reason
> >>> (like measurable and relevant performance impact), I doubt that's
> >>> the case here.
> >> Your skeptical-ability of arbitrary limitations has become very
> >> clear in the last few hours... ;-) I guess I'm indifferent about
> >> it... From reading your gssd patch set, it is a bit more artful
> >> not to use fixed size buffers but again, I'm indifferent... That
> >> said... if patches appear removing these fixed buffers they
> >> definitely would be considered... 
> >>
> >>>
> >>> It's up to the maintainer though, I just wanted to point it out :)
> >> My understanding these patches were needed to make nfs-utils
> >> compatible with the musl c-library. That is the case, correct? 

Yes. It is because musl FILE implementation uses writev() and readv()
with multiple buffers, and the kernel side does not handle that.

Also, the write side was very brittle even with glibc, it probably was
broken with large messages.

> > The fread/fwrite removal seems reasonable, yes. The removal of the
> > readline() function though (which could be implemented using normal
> > read/malloc/realloc) seems less so.....IMHO.
> > 
> Patches welcome! 8-)

Normally having upper limits is also a security feature. It means the
other end cannot force the client code to receive so large messages
that it runs out of memory. Though, in this case the other end is
kernel and to be trusted.

Though, it probably helps catch potential errors. The messages from
kernel really cannot exceed that length. If they do, it's an error
anyway. The message structure pretty much ensures this.

In my opinion the dynamic allocation is a step backward, rather then
forwards. It adds potential failure (out of memory), is not required,
and it does not add any features either.

IMHO, "just because it used to be so" is a bad excuse. And it would
just cause additional code making harder to debug and easier to fail.
Why add complexity when it can be done simpler?

just my five cents,
Timo