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.
