On Wed, Jan 21, 2015 at 5:42 PM, enh <[email protected]> wrote:
> On Wed, Jan 21, 2015 at 3:04 AM, Martin Pieuchot <[email protected]>
> wrote:
>> Hello Elliott,
>>
>> On 20/01/15(Tue) 16:15, enh wrote:
>>> that patch wasn't setting the _flags right on error or eof.
>>
>> Thanks! Below is a version of your diff with some tweaks to match the
>> recent changes done in -current.
>>
>> In your original post you've shown some test numbers. Does it mean that
>> you have a test program for fread(3)? If so do you think it would fit as
>> test in OpenBSD's regress framework?
>
> we have over a thousand C library tests and a small number of
> benchmarks in bionic/tests and bionic/benchmarks in the Android tree,
> all BSD-licensed. (though this only amounts to 42% coverage.)
>
>> I'd suggest you to check the setup of your mail client, apparently it
>> eats all the tabs and makes impossible to apply your diffs correctly :)
>
> afaik, there's nothing to can do to stop gmail eating tabs.
Check out devio.us if you need a shell account, to send diffs to OpenBSD.
>
>> Index: fread.c
>> ===================================================================
>> RCS file: /home/ncvs/src/lib/libc/stdio/fread.c,v
>> retrieving revision 1.13
>> diff -u -p -r1.13 fread.c
>> --- fread.c 8 Dec 2014 20:40:53 -0000 1.13
>> +++ fread.c 21 Jan 2015 10:54:56 -0000
>> @@ -37,18 +37,19 @@
>> #include <errno.h>
>> #include "local.h"
>>
>> +#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
>
> you don't want to use the MIN from <sys/param.h>? many files in libc
> already do. (though admittedly fvwrite.c defines its own, but that
> seems like a bug.)
>
>> #define MUL_NO_OVERFLOW (1UL << (sizeof(size_t) * 4))
>>
>> size_t
>> fread(void *buf, size_t size, size_t count, FILE *fp)
>> {
>> - size_t resid;
>> - char *p;
>> - int r;
>> + size_t desired_total;
>> size_t total;
>> + char *dst;
>>
>> /*
>> - * Extension: Catch integer overflow
>> + * Extension: Catch integer overflow.
>> */
>> if ((size >= MUL_NO_OVERFLOW || count >= MUL_NO_OVERFLOW) &&
>> size > 0 && SIZE_MAX / size < count) {
>> @@ -57,47 +58,79 @@ fread(void *buf, size_t size, size_t cou
>> return (0);
>> }
>>
>> + desired_total = count * size;
>> + total = desired_total;
>> +
>> /*
>> * ANSI and SUSv2 require a return value of 0 if size or count are 0.
>> */
>> - if ((resid = count * size) == 0)
>> + if (total == 0)
>> return (0);
>> +
>> FLOCKFILE(fp);
>> _SET_ORIENTATION(fp, -1);
>> +
>> + /* XXX: how can this ever happen?! */
>> if (fp->_r < 0)
>> fp->_r = 0;
>> - total = resid;
>> - p = buf;
>>
>> - if ((fp->_flags & __SNBF) != 0) {
>> +
>> + /*
>> + * Ensure _bf._size is valid.
>> + */
>> + if (fp->_bf._base == NULL)
>> + __smakebuf(fp);
>> +
>> + dst = buf;
>> +
>> + while (total > 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.
>> + * Copy data out of the buffer.
>> */
>> - while (resid > 0 && (r = (*fp->_read)(fp->_cookie, p,
>> resid)) > 0) {
>> - p += r;
>> - resid -= r;
>> - }
>> - FUNLOCKFILE(fp);
>> - return ((total - resid) / size);
>> + size_t buffered_bytes = MINIMUM(fp->_r, total);
>> +
>> + memcpy(dst, fp->_p, buffered_bytes);
>> + fp->_p += buffered_bytes;
>> + fp->_r -= buffered_bytes;
>> + dst += buffered_bytes;
>> + total -= buffered_bytes;
>> +
>> + /*
>> + * Are we done?
>> + */
>> + if (total == 0)
>> + goto out;
>> +
>> + /*
>> + * Do we have so much more to read that we should
>> + * avoid copying it through the buffer?
>> + */
>> + if (total > (size_t) fp->_bf._size)
>> + break;
>> +
>> + /*
>> + * Less than a buffer to go, so refill the buffer and
>> + * go around the loop again.
>> + */
>> + if (__srefill(fp))
>> + goto out;
>> }
>>
>> - while (resid > (r = fp->_r)) {
>> - (void)memcpy((void *)p, (void *)fp->_p, (size_t)r);
>> - fp->_p += r;
>> - /* fp->_r = 0 ... done in __srefill */
>> - p += r;
>> - resid -= r;
>> - if (__srefill(fp)) {
>> - /* no more input: return partial result */
>> - FUNLOCKFILE(fp);
>> - return ((total - resid) / size);
>> + /*
>> + * Read directly into the caller's buffer.
>> + */
>> + while (total > 0) {
>> + ssize_t bytes_read = (*fp->_read)(fp->_cookie, dst, total);
>> +
>> + if (bytes_read <= 0) {
>> + fp->_flags = (fp->_r == 0) ? __SEOF : __SERR;
>> + break;
>> }
>> + dst += bytes_read;
>> + total -= bytes_read;
>> }
>> - (void)memcpy((void *)p, (void *)fp->_p, resid);
>> - fp->_r -= resid;
>> - fp->_p += resid;
>> +
>> +out:
>> FUNLOCKFILE(fp);
>> - return (count);
>> + return ((desired_total - total) / size);
>> }
>
--
This message is strictly personal and the opinions expressed do not
represent those of my employers, either past or present.
