On Fri, 14 Dec 2018 08:45:31 +0100, Sebastien Marie wrote:
> I like a lot the way it is done. Using a copy of FILE argument with
> _bf._size changed to request read size is great.
>
> and as juanfra@ said, it solves mercurial problem (I tested it too).
>
> some comments inlined.
>
> > Index: lib/libc/stdio/fread.c
> > ===================================================================
> > RCS file: /cvs/src/lib/libc/stdio/fread.c,v
> > retrieving revision 1.15
> > diff -u -p -u -r1.15 fread.c
> > --- lib/libc/stdio/fread.c 21 Sep 2016 04:38:56 -0000 1.15
> > +++ lib/libc/stdio/fread.c 13 Dec 2018 17:24:34 -0000
> > @@ -69,18 +69,39 @@ fread(void *buf, size_t size, size_t cou
> > total = resid;
> > p = buf;
> >
> > + /*
> > + * If we're unbuffered we know that the buffer in fp is empty so
> > + * we can read directly into buf. This is much faster than a
> > + * series of one byte reads into fp->_nbuf.
> > + */
> > if ((fp->_flags & __SNBF) != 0) {
> > - /*
> > - * We know if we're unbuffered that our buffer is empty, so
> > - * we can just read directly. This is much faster than the
> > - * loop below which will perform a series of one byte reads.
> > - */
> > - while (resid > 0 && (r = (*fp->_read)(fp->_cookie, p, resid)) >
> 0) {
> > - p += r;
> > - resid -= r;
> > + FILE fake;
> > + struct __sfileext fakeext;
> > +
> > + _FILEEXT_SETUP(&fake, &fakeext);
> > + /* copy the important variables */
> > + fake._flags = fp->_flags;
> > + fake._file = fp->_file;
> > + fake._cookie = fp->_cookie;
> > + fake._read = fp->_read;
>
> in some cases (but it might be an ill argument passed to fread(): see
> "if not already reading, have to be reading and writing"), __srefill()
> could call __sflush(), and it will call _write handler (which is
> uninitialized data). should we copying others handlers too ?
Perhaps. For that code path to be safe we'd also need to initialize
fp->_p to the same value as fp->_bf._base. It's not really possible
to flush any data since we've replaced the buffer but since we are
unbuffered anyway there is nothing to flush.
> > +
> > + /* set up the buffer */
> > + fake._bf._base = buf;
>
> does a test for ill arguments (like buf == NULL) should be done ?
> else __srefill() will call __smakebuf(), and it will set:
>
> fp->_bf._base = fp->_p = fp->_nbuf;
>
> using _nbuf which is uninitialized data.
If buf is NULL then we should just skip this optimization and let
fread(3) deference NULL in the memcpy() call.
> > + fake._bf._size = total;
> > + fake._lbfsize = 0; /* not line buffered */
> > +
>
> Globally, I think `fake' should be properly initialized by doing a
> memcpy() with `fp' data. And next, override values to copte with buffer
> size. This way, it will take care of all ill cases, and no uninitialized
> data will be used.
I was initially going to do that but decided that since vfprintf.c
just initialized things manually to follow suit. Another option
is to just use fp directly and reset fp->bf and fp->p when we are
done.
> > + while (resid > 0) {
> > + if (__srefill(&fake)) {
> > + /* no more input: return partial result */
> > + count = (total - resid) / size;
> > + fp->_flags |= (fake._flags & (__SERR|__SEOF));
>
> Initially, I wonder why you copied back only __SERR|__SEOF to _flags
> (__srefill could change few others flags), but as _flags is sole
> parameter to be copied back it makes sens.
>
> > + break;
> > + }
> > + fake._p += fake._r;
> > + resid -= fake._r;
> > }
> > FUNLOCKFILE(fp);
> > - return ((total - resid) / size);
> > + return (count);
> > }
> >
> > while (resid > (r = fp->_r)) {
After thinking about this a bit more I believe it best to just use
the existing FILE * and swap in the buffer temporarily. There's
no need to store the value of the old buffer; since we are unbuffered
it can only point to _nbuf.
This is simpler and should address all of your concerns.
- todd
Index: lib/libc/stdio/fread.c
===================================================================
RCS file: /cvs/src/lib/libc/stdio/fread.c,v
retrieving revision 1.15
diff -u -p -u -r1.15 fread.c
--- lib/libc/stdio/fread.c 21 Sep 2016 04:38:56 -0000 1.15
+++ lib/libc/stdio/fread.c 14 Dec 2018 17:48:46 -0000
@@ -69,18 +69,33 @@ fread(void *buf, size_t size, size_t cou
total = resid;
p = buf;
- if ((fp->_flags & __SNBF) != 0) {
- /*
- * We know if we're unbuffered that our buffer is empty, so
- * we can just read directly. This is much faster than the
- * loop below which will perform a series of one byte reads.
- */
- while (resid > 0 && (r = (*fp->_read)(fp->_cookie, p, resid)) >
0) {
- p += r;
- resid -= r;
+ /*
+ * If we're unbuffered we know that the buffer in fp is empty so
+ * we can read directly into buf. This is much faster than a
+ * series of one byte reads into fp->_nbuf.
+ */
+ if ((fp->_flags & __SNBF) != 0 && buf != NULL) {
+ /* set up the buffer */
+ fp->_bf._base = fp->_p = buf;
+ fp->_bf._size = total;
+
+ while (resid > 0) {
+ if (__srefill(fp)) {
+ /* no more input: return partial result */
+ count = (total - resid) / size;
+ break;
+ }
+ fp->_p += fp->_r;
+ resid -= fp->_r;
}
+
+ /* restore the old buffer (see __smakebuf) */
+ fp->_bf._base = fp->_p = fp->_nbuf;
+ fp->_bf._size = 1;
+ fp->_r = 0;
+
FUNLOCKFILE(fp);
- return ((total - resid) / size);
+ return (count);
}
while (resid > (r = fp->_r)) {
