Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock

Stefan Beller <sbeller@google.com> writes:

s/Add/add/;

I am hoping that you are at least not spinning---i.e. do a poll first to make sure there is at least some progress to be made before calling this.

In order to ensure that this is not breaking the normal case, I had to look at the implementation of xread() to see it behaves identically when the flags is not passed. That one-time review burden implies that this is adding a maintenance burden to keep this copied function in sync.

We should also handle EWOULDBLOCK not just EAGAIN.

More importantly, I am not sure if this helper is even necessary.

Looking at xread (reproduced in its full glory):

/*
 * xread() is the same a read(), but it automatically restarts read()
 * operations with a recoverable error (EAGAIN and EINTR). xread()
 * DOES NOT GUARANTEE that "len" bytes is read even if the data is available.
 */
ssize_t xread(int fd, void *buf, size_t len)
{
	ssize_t nr;
	if (len > MAX_IO_SIZE)
	    len = MAX_IO_SIZE;
	while (1) {
		nr = read(fd, buf, len);
		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
			continue;
		return nr;
	}
}

Are we doing the right thing for EAGAIN here?

Now, man read(2) says this:

       EAGAIN 
		The file descriptor fd refers to a file other than a
                socket and has been marked nonblocking (O_NONBLOCK),
                and the read would block.

       EAGAIN or EWOULDBLOCK
		The file descriptor fd refers to a socket and has
                been marked nonblocking (O_NONBLOCK), and the read
                would block.  POSIX.1-2001 allows either error to be
                returned for this case, and does not require these
                con‐ stants to have the same value, so a portable
                application should check for both possibilities.

If the fd is not marked nonblocking, then we will not get EAGAIN (or EWOULDBLOCK).

When fd is set to nonblocking, the current xread() spins if read() says that the operation would block. What would we achieve by spinning ourselves here, though? The caller must have set the fd to be nonblocking for a reason, and that reason cannot be "if the data is not ready, we do not have anything better than just spinning for new data to arrive"---if so, the caller wouldn't have set the fd to be nonblocking in the first place.

Even if there is such a stupid caller that only wants us to loop, because we explicitly allow xread() to return a short read, all of our callers must call it in a loop if they know how much they want to read. We can just return from here and let them loop around us.

And your new caller that does O_NONBLOCK wants to do more than looping upon EWOULDBLOCK. It certainly would not want us to loop here.

So I wonder if you can just O_NONBLOCK the fd and use the usual strbuf_read(), i.e. without any change in this patch, and update xread() to _unconditionally_ return when read(2) says EAGAIN or EWOULDBLOCK.

What would that break?