Re: [PATCH] Updated Comm::Read I/O API

On 06/04/2014 09:10 AM, Amos Jeffries wrote:

> * Comm::Read() which accepts the Comm::Connection pointer for the
> socket to read on and an AsyncCall callback to be run when read is
> ready. The Job is responsible for separately initiating read(2) or
> alternative action when that callback is run.
>
> * Comm::ReadNow() which accepts an SBuf buffer and a CommIoCbParams
> initialized to contain the Comm::Connection pointer for the socket to
> read on which will be filled out with result flag, xerrno, and size.
> This synchronously performs read(2) operations to append bytes to the
> provided buffer. It returns a comm_err_t flag for use in determining how
> to handle the results and signalling one of OK, INPROGRESS, ERROR,
> ERR_CLOSING as having happened.

> +inline void comm_read(const Comm::ConnectionPointer &conn, char *buf, int len, AsyncCall::Pointer &callback)
> +{
> + assert(buf != NULL);

but:

> commHalfClosedCheck(void *)
> {
...
> comm_read(c, NULL, 0, call);

Please fix and double check that no other comm_read calls are using a
NULL buffer. If you cannot be sure, let's not enforce this to avoid
breaking working code.

> + if (retval > 0) { // data read most common case
...
> + fd_bytes(params.conn->fd, retval, FD_READ);

> + /* Note - read 0 == socket EOF, which is a valid read */
> + if (retval >= 0) {
> + fd_bytes(fd, retval, FD_READ);

Please be consistent. Since bytes accounting is not your patch focus, I
suggest calling fd_bytes() for zero returned value to avoid a subtle
irrelevant change.

> + // if the caller did not supply a buffer, just schedule callback

Please detail along these lines:

  // without a buffer, just call back and the caller will ReadNow()

> + /* For legacy callers : Attempt a read */

Please add something like "Keep in sync with Comm::ReadNow()!"

> + debugs(5, 3, "SBuf " << params.conn << ", size " << sz << ", retval " << retval << ", errno " << params.xerrno);

> + debugs(5, 3, "char FD " << fd << ", size " << ccb->size << ", retval " << retval << ", errno " << errno);

Please remove the "SBuf " and "char " prefixes. These lines are about
Comm I/O, these specific prefixes do not make much sense here, and
debugs() will supply the right context.

> + SBuf::size_type sz = buf.spaceSize();

Please make const if possible.

> + char *b = buf.rawSpace(sz);

Please rename "b" to "space", "rawSpace", or something like that to ease
reading/searching.

> + int retval = FD_READ_METHOD(params.conn->fd, b, sz);

Please make constant.

> + } else if (retval == 0) { // remote closure (somewhat less) common
> + // Note - read 0 == socket EOF, which is a valid read.
> + params.flag = COMM_ERR_CLOSING;

It is a bad idea to overload COMM_ERR_CLOSING like this! The reaction to
COMM_ERR_CLOSING in the callback is very different to the reaction to
EOF! And EOF is not really an error. If you think this case deserves a
dedicated flag, add COMM_EOF instead.

> + debugs(5, 3, params.conn << " COMM_ERROR: " << xstrerror());

Supply params.xerrno. The errno global may have changed.

> /* Bail out quickly on COMM_ERR_CLOSING - close handlers will tidy up */
> -
> if (io.flag == COMM_ERR_CLOSING) {
> - debugs(33,5, HERE << io.conn << " closing Bailout.");
> + debugs(33,5, io.conn << " closing Bailout.");
> return;
> }

Avoid non-changes.

> - /*
> - * Don't reset the timeout value here. The timeout value will be
> - * set to Config.Timeout.request by httpAccept() and
> - * clientWriteComplete(), and should apply to the request as a
> - * whole, not individual read() calls. Plus, it breaks our
> - * lame half-close detection
> - */

Are these comments no longer correct? Restore if unsure.

I believe the above adjustments can be made without another round of
reviews.

FWIW, I was not able to verify that the moved Comm code and the reshaped
ConnStateData::clientReadRequest() code contained only the intentional
changes.

Thank you,

Alex.
Received on Wed Jun 04 2014 - 17:16:16 MDT