Re: [PATCH] proc_file_read() (Was: Re: proc_file_read() question)

[Martin Wilck]

On Tue, 26 Jun 2001, Jonathan Lundell wrote:

> I use the hack myself, to implement a record-oriented file where the
> file position is a record number. I could probably live with
> PAGE_SIZE, but the current hack works fine with start bigger than
> that, and it's possible that someone counts on it.

Ok, let's use PAGE_OFFSET instead of PAGE_SIZE, then (see new patch
below).

Unless I'm mislead, legitimate values of "start" as a pointer are always
larger than that, and I can hardly imagin e a case where the "unsigned
int" value of start must be greater than PAGE_OFFSET.

I insist that relying on the comparison of two pointers is the wrong
thing. If (as you suggest) the major use of "start" has migrated from the
original intention to that of the "hack", this should be reflected
in the interface by making the "start" parameter to read_proc ()
an unsigned long. Everything else is misleading and error-prone.
For now, "start" is a char* and should be treated as such.

> But if you're allocating your own buffer, you'd probably be better
> off writing your own file ops, and not using the default
> proc_file_read() at all. At the very least you'd save a redundant
> __get_free_page/free_page pair.

That's right, but nevertheless (repeat) comparing "start" and "page" is
wrong.

Regards,
Martin

-- 
Martin Wilck     <[e-mail]>
FSC EP PS DS1, Paderborn      Tel. [phone]

--- linux-2.4.5/fs/proc/generic.c	Mon Jun 25 13:46:26 2001
+++ 2.4.5mw/fs/proc/generic.c	Wed Jun 27 11:22:14 2001
@@ -104,14 +104,14 @@
  		 * return the bytes, and set `start' to the desired offset
  		 * as an unsigned int. - [e-mail]
 		 */
- 		n -= copy_to_user(buf, start < page ? page : start, n);
+ 		n -= copy_to_user(buf, (unsigned long) start < PAGE_OFFSET ? page : start, n);
 		if (n == 0) {
 			if (retval == 0)
 				retval = -EFAULT;
 			break;
 		}

-		*ppos += start < page ? (long)start : n; /* Move down the file */
+		*ppos += (unsigned long) start < PAGE_OFFSET ? (unsigned long) start : n; /* Move down the file */
 		nbytes -= n;
 		buf += n;
 		retval += n;