From: Radoslaw Zarzynski <rzarzyns@redhat.com>
Date: Tue, 9 Nov 2021 11:36:31 +0000 (+0000)
Subject: msg/async: remove the misleading error description on read failures
X-Git-Tag: v18.0.0~1290^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=fecfdd464c23275ebc0d29bb3713aa6d3e439a27;p=ceph.git

msg/async: remove the misleading error description on read failures

Before this patch we were calling `cpp_strerror` on the error code
returned by messenger's low-level methods for reading from socket.
Unfortunately, it leaded to misleading debugs like:

  ```
  (...).read_bulk peer close file descriptor 28
  (...).read_until read failed
  (...).handle_read_frame_preamble_main read frame preamble failed r=-1 ((1) Operation not permitted)
  ```

The problem is that these lower layers simply don't adhere to the
POSIX-like, ernno convention for reporting problems. Therefore,
`-1` doesn't really mean _Operation not permitted_. The snippets
bellow show the actual convention.

  ```
  /* return -1 means `fd` occurs error or closed, it should be closed
   * return 0 means EAGAIN or EINTR */
  ssize_t AsyncConnection::read_bulk(char *buf, unsigned len)
  {
    ssize_t nread;
   again:
    nread = cs.read(buf, len);
    if (nread < 0) {
      if (nread == -EAGAIN) {
        nread = 0;
      } else if (nread == -EINTR) {
        goto again;
      } else {
        ldout(async_msgr->cct, 1) << __func__ << " reading from fd=" << cs.fd()
                            << " : "<< nread << " " << strerror(nread) << dendl;
        return -1;
      }
    } else if (nread == 0) {
      ldout(async_msgr->cct, 1) << __func__ << " peer close file descriptor "
                                << cs.fd() << dendl;
      return -1;
    }
    return nread;
  }

  size_t AsyncConnection::read_until(unsigned len, char *p)
  {
    // ...
    recv_end = recv_start = 0;
    /* nothing left in the prefetch buffer */
    if (left > (uint64_t)recv_max_prefetch) {
      /* this was a large read, we don't prefetch for these */
      do {
        r = read_bulk(p+state_offset, left);
        ldout(async_msgr->cct, 25) << __func__ << " read_bulk left is " << left << " got " << r << dendl;
        if (r < 0) {
          ldout(async_msgr->cct, 1) << __func__ << " read failed" << dendl;
          return -1;
        } else if (r == static_cast<int>(left)) {
          state_offset = 0;
          return 0;
        }
        state_offset += r;
        left -= r;
      } while (r > 0);
    } else {
    // ...
  }
  CtPtr ProtocolV2::read_frame() {
    if (state == CLOSED) {
      return nullptr;
    }

    ldout(cct, 20) << __func__ << dendl;
    rx_preamble.clear();
    rx_epilogue.clear();
    rx_segments_data.clear();

    return READ(rx_frame_asm.get_preamble_onwire_len(),
                handle_read_frame_preamble_main);
  }

  CtPtr ProtocolV2::handle_read_frame_preamble_main(rx_buffer_t &&buffer, int r) {
    ldout(cct, 20) << __func__ << " r=" << r << dendl;

    if (r < 0) {
      ldout(cct, 1) << __func__ << " read frame preamble failed r=" << r
                    << " (" << cpp_strerror(r) << ")" << dendl;
      return _fault();
    }
    // ...
  }
  ```

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
---

diff --git a/src/msg/async/ProtocolV2.cc b/src/msg/async/ProtocolV2.cc
index a176fc2c808a..2916f073e637 100644
--- a/src/msg/async/ProtocolV2.cc
+++ b/src/msg/async/ProtocolV2.cc
@@ -1083,7 +1083,7 @@ CtPtr ProtocolV2::handle_read_frame_preamble_main(rx_buffer_t &&buffer, int r) {
 
   if (r < 0) {
     ldout(cct, 1) << __func__ << " read frame preamble failed r=" << r
-                  << " (" << cpp_strerror(r) << ")" << dendl;
+                  << dendl;
     return _fault();
   }