From 3523bdc1d6abb74fdc0a572c9fecd7f2633ec4b9 Mon Sep 17 00:00:00 2001

From: John Snow <jsnow@redhat.com>

Date: Wed, 27 Mar 2019 17:22:57 +0100

Subject: [PATCH 119/163] nbd: generalize usage of nbd_read

RH-Author: John Snow <jsnow@redhat.com>

Message-id: <20190327172308.31077-45-jsnow@redhat.com>

Patchwork-id: 85205

O-Subject: [RHEL-7.7 qemu-kvm-rhev PATCH 44/55] nbd: generalize usage of nbd_read

Bugzilla: 1691009

RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

RH-Acked-by: Max Reitz <mreitz@redhat.com>

RH-Acked-by: Miroslav Rezanina <mrezanin@redhat.com>

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

We generally do very similar things around nbd_read: error_prepend

specifying what we have tried to read, and be_to_cpu conversion of

integers.

So, it seems reasonable to move common things to helper functions,

which:

1. simplify code a bit

2. generalize nbd_read error descriptions, all starting with

   "Failed to read"

3. make it more difficult to forget to convert things from BE

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

Message-Id: <20190128165830.165170-1-vsementsov@virtuozzo.com>

[eblake: rename macro to DEF_NBD_READ_N and formatting tweaks;

checkpatch has false positive complaint]

Signed-off-by: Eric Blake <eblake@redhat.com>

(cherry picked from commit e6798f06a67a25def45a6636259de38cc38f1414)

Signed-off-by: John Snow <jsnow@redhat.com>

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>

---

 block/nbd-client.c  |  5 ++-

 include/block/nbd.h | 32 +++++++++++++++++--

 nbd/client.c        | 88 ++++++++++++++++++-----------------------------------

 nbd/common.c        |  2 +-

 nbd/server.c        | 27 ++++++----------

 5 files changed, 71 insertions(+), 83 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c

index 8135396..5c97052 100644

--- a/block/nbd-client.c

+++ b/block/nbd-client.c

@@ -337,10 +337,9 @@ static int nbd_co_receive_offset_data_payload(NBDClientSession *s,

         return -EINVAL;

     }

-    if (nbd_read(s->ioc, &offset, sizeof(offset), errp) < 0) {

+    if (nbd_read64(s->ioc, &offset, "OFFSET_DATA offset", errp) < 0) {

         return -EIO;

     }

-    be64_to_cpus(&offset);

     data_size = chunk->length - sizeof(offset);

     assert(data_size);

@@ -387,7 +386,7 @@ static coroutine_fn int nbd_co_receive_structured_payload(

     }

     *payload = g_new(char, len);

-    ret = nbd_read(s->ioc, *payload, len, errp);

+    ret = nbd_read(s->ioc, *payload, len, "structured payload", errp);

     if (ret < 0) {

         g_free(*payload);

         *payload = NULL;

diff --git a/include/block/nbd.h b/include/block/nbd.h

index 4faf394..96cfb1d 100644

--- a/include/block/nbd.h

+++ b/include/block/nbd.h

@@ -23,6 +23,7 @@

 #include "qapi/qapi-types-block.h"

 #include "io/channel-socket.h"

 #include "crypto/tlscreds.h"

+#include "qapi/error.h"

 /* Handshake phase structs - this struct is passed on the wire */

@@ -336,11 +337,38 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,

  * Reads @size bytes from @ioc. Returns 0 on success.

  */

 static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,

-                           Error **errp)

+                           const char *desc, Error **errp)

 {

-    return qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;

+    int ret = qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;

+

+    if (ret < 0) {

+        if (desc) {

+            error_prepend(errp, "Failed to read %s: ", desc);

+        }

+        return -1;

+    }

+

+    return 0;

+}

+

+#define DEF_NBD_READ_N(bits)                                            \

+static inline int nbd_read##bits(QIOChannel *ioc,                       \

+                                 uint##bits##_t *val,                   \

+                                 const char *desc, Error **errp)        \

+{                                                                       \

+    if (nbd_read(ioc, val, sizeof(*val), desc, errp) < 0) {             \

+        return -1;                                                      \

+    }                                                                   \

+    *val = be##bits##_to_cpu(*val);                                     \

+    return 0;                                                           \

 }

+DEF_NBD_READ_N(16) /* Defines nbd_read16(). */

+DEF_NBD_READ_N(32) /* Defines nbd_read32(). */

+DEF_NBD_READ_N(64) /* Defines nbd_read64(). */

+

+#undef DEF_NBD_READ_N

+

 static inline bool nbd_reply_is_simple(NBDReply *reply)

 {

     return reply->magic == NBD_SIMPLE_REPLY_MAGIC;

diff --git a/nbd/client.c b/nbd/client.c

index 8a083c2..10a52ad 100644

--- a/nbd/client.c

+++ b/nbd/client.c

@@ -113,8 +113,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,

                                     NBDOptionReply *reply, Error **errp)

 {

     QEMU_BUILD_BUG_ON(sizeof(*reply) != 20);

-    if (nbd_read(ioc, reply, sizeof(*reply), errp) < 0) {

-        error_prepend(errp, "failed to read option reply: ");

+    if (nbd_read(ioc, reply, sizeof(*reply), "option reply", errp) < 0) {

         nbd_send_opt_abort(ioc);

         return -1;

     }

@@ -166,8 +165,8 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,

             goto cleanup;

         }

         msg = g_malloc(reply->length + 1);

-        if (nbd_read(ioc, msg, reply->length, errp) < 0) {

-            error_prepend(errp, "failed to read option error %" PRIu32

+        if (nbd_read(ioc, msg, reply->length, NULL, errp) < 0) {

+            error_prepend(errp, "Failed to read option error %" PRIu32

                           " (%s) message: ",

                           reply->type, nbd_rep_lookup(reply->type));

             goto cleanup;

@@ -284,12 +283,10 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,

         nbd_send_opt_abort(ioc);

         return -1;

     }

-    if (nbd_read(ioc, &namelen, sizeof(namelen), errp) < 0) {

-        error_prepend(errp, "failed to read option name length: ");

+    if (nbd_read32(ioc, &namelen, "option name length", errp) < 0) {

         nbd_send_opt_abort(ioc);

         return -1;

     }

-    namelen = be32_to_cpu(namelen);

     len -= sizeof(namelen);

     if (len < namelen) {

         error_setg(errp, "incorrect option name length");

@@ -298,8 +295,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,

     }

     local_name = g_malloc(namelen + 1);

-    if (nbd_read(ioc, local_name, namelen, errp) < 0) {

-        error_prepend(errp, "failed to read export name: ");

+    if (nbd_read(ioc, local_name, namelen, "export name", errp) < 0) {

         nbd_send_opt_abort(ioc);

         goto out;

     }

@@ -307,8 +303,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,

     len -= namelen;

     if (len) {

         local_desc = g_malloc(len + 1);

-        if (nbd_read(ioc, local_desc, len, errp) < 0) {

-            error_prepend(errp, "failed to read export description: ");

+        if (nbd_read(ioc, local_desc, len, "export description", errp) < 0) {

             nbd_send_opt_abort(ioc);

             goto out;

         }

@@ -410,13 +405,11 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,

             nbd_send_opt_abort(ioc);

             return -1;

         }

-        if (nbd_read(ioc, &type, sizeof(type), errp) < 0) {

-            error_prepend(errp, "failed to read info type: ");

+        if (nbd_read16(ioc, &type, "info type", errp) < 0) {

             nbd_send_opt_abort(ioc);

             return -1;

         }

         len -= sizeof(type);

-        type = be16_to_cpu(type);

         switch (type) {

         case NBD_INFO_EXPORT:

             if (len != sizeof(info->size) + sizeof(info->flags)) {

@@ -425,18 +418,14 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,

                 nbd_send_opt_abort(ioc);

                 return -1;

             }

-            if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {

-                error_prepend(errp, "failed to read info size: ");

+            if (nbd_read64(ioc, &info->size, "info size", errp) < 0) {

                 nbd_send_opt_abort(ioc);

                 return -1;

             }

-            info->size = be64_to_cpu(info->size);

-            if (nbd_read(ioc, &info->flags, sizeof(info->flags), errp) < 0) {

-                error_prepend(errp, "failed to read info flags: ");

+            if (nbd_read16(ioc, &info->flags, "info flags", errp) < 0) {

                 nbd_send_opt_abort(ioc);

                 return -1;

             }

-            info->flags = be16_to_cpu(info->flags);

             trace_nbd_receive_negotiate_size_flags(info->size, info->flags);

             break;

@@ -447,27 +436,23 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,

                 nbd_send_opt_abort(ioc);

                 return -1;

             }

-            if (nbd_read(ioc, &info->min_block, sizeof(info->min_block),

-                         errp) < 0) {

-                error_prepend(errp, "failed to read info minimum block size: ");

+            if (nbd_read32(ioc, &info->min_block, "info minimum block size",

+                           errp) < 0) {

                 nbd_send_opt_abort(ioc);

                 return -1;

             }

-            info->min_block = be32_to_cpu(info->min_block);

             if (!is_power_of_2(info->min_block)) {

                 error_setg(errp, "server minimum block size %" PRIu32

                            " is not a power of two", info->min_block);

                 nbd_send_opt_abort(ioc);

                 return -1;

             }

-            if (nbd_read(ioc, &info->opt_block, sizeof(info->opt_block),

-                         errp) < 0) {

-                error_prepend(errp,

-                              "failed to read info preferred block size: ");

+            if (nbd_read32(ioc, &info->opt_block, "info preferred block size",

+                           errp) < 0)

+            {

                 nbd_send_opt_abort(ioc);

                 return -1;

             }

-            info->opt_block = be32_to_cpu(info->opt_block);

             if (!is_power_of_2(info->opt_block) ||

                 info->opt_block < info->min_block) {

                 error_setg(errp, "server preferred block size %" PRIu32

@@ -475,13 +460,12 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,

                 nbd_send_opt_abort(ioc);

                 return -1;

             }

-            if (nbd_read(ioc, &info->max_block, sizeof(info->max_block),

-                         errp) < 0) {

-                error_prepend(errp, "failed to read info maximum block size: ");

+            if (nbd_read32(ioc, &info->max_block, "info maximum block size",

+                           errp) < 0)

+            {

                 nbd_send_opt_abort(ioc);

                 return -1;

             }

-            info->max_block = be32_to_cpu(info->max_block);

             if (info->max_block < info->min_block) {

                 error_setg(errp, "server maximum block size %" PRIu32

                            " is not valid", info->max_block);

@@ -731,14 +715,13 @@ static int nbd_receive_one_meta_context(QIOChannel *ioc,

         return -1;

     }

-    if (nbd_read(ioc, &local_id, sizeof(local_id), errp) < 0) {

+    if (nbd_read32(ioc, &local_id, "context id", errp) < 0) {

         return -1;

     }

-    local_id = be32_to_cpu(local_id);

     reply.length -= sizeof(local_id);

     local_name = g_malloc(reply.length + 1);

-    if (nbd_read(ioc, local_name, reply.length, errp) < 0) {

+    if (nbd_read(ioc, local_name, reply.length, "context name", errp) < 0) {

         g_free(local_name);

         return -1;

     }

@@ -896,11 +879,9 @@ static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,

         return -EINVAL;

     }

-    if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) {

-        error_prepend(errp, "Failed to read initial magic: ");

+    if (nbd_read64(ioc, &magic, "initial magic", errp) < 0) {

         return -EINVAL;

     }

-    magic = be64_to_cpu(magic);

     trace_nbd_receive_negotiate_magic(magic);

     if (magic != NBD_INIT_MAGIC) {

@@ -908,11 +889,9 @@ static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,

         return -EINVAL;

     }

-    if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) {

-        error_prepend(errp, "Failed to read server magic: ");

+    if (nbd_read64(ioc, &magic, "server magic", errp) < 0) {

         return -EINVAL;

     }

-    magic = be64_to_cpu(magic);

     trace_nbd_receive_negotiate_magic(magic);

     if (magic == NBD_OPTS_MAGIC) {

@@ -920,11 +899,9 @@ static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,

         uint16_t globalflags;

         bool fixedNewStyle = false;

-        if (nbd_read(ioc, &globalflags, sizeof(globalflags), errp) < 0) {

-            error_prepend(errp, "Failed to read server flags: ");

+        if (nbd_read16(ioc, &globalflags, "server flags", errp) < 0) {

             return -EINVAL;

         }

-        globalflags = be16_to_cpu(globalflags);

         trace_nbd_receive_negotiate_server_flags(globalflags);

         if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {

             fixedNewStyle = true;

@@ -992,17 +969,13 @@ static int nbd_negotiate_finish_oldstyle(QIOChannel *ioc, NBDExportInfo *info,

 {

     uint32_t oldflags;

-    if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {

-        error_prepend(errp, "Failed to read export length: ");

+    if (nbd_read64(ioc, &info->size, "export length", errp) < 0) {

         return -EINVAL;

     }

-    info->size = be64_to_cpu(info->size);

-    if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) {

-        error_prepend(errp, "Failed to read export flags: ");

+    if (nbd_read32(ioc, &oldflags, "export flags", errp) < 0) {

         return -EINVAL;

     }

-    oldflags = be32_to_cpu(oldflags);

     if (oldflags & ~0xffff) {

         error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);

         return -EINVAL;

@@ -1079,17 +1052,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,

         }

         /* Read the response */

-        if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) {

-            error_prepend(errp, "Failed to read export length: ");

+        if (nbd_read64(ioc, &info->size, "export length", errp) < 0) {

             return -EINVAL;

         }

-        info->size = be64_to_cpu(info->size);

-        if (nbd_read(ioc, &info->flags, sizeof(info->flags), errp) < 0) {

-            error_prepend(errp, "Failed to read export flags: ");

+        if (nbd_read16(ioc, &info->flags, "export flags", errp) < 0) {

             return -EINVAL;

         }

-        info->flags = be16_to_cpu(info->flags);

         break;

     case 0: /* oldstyle, parse length and flags */

         if (*info->name) {

@@ -1379,7 +1348,7 @@ static int nbd_receive_simple_reply(QIOChannel *ioc, NBDSimpleReply *reply,

     assert(reply->magic == NBD_SIMPLE_REPLY_MAGIC);

     ret = nbd_read(ioc, (uint8_t *)reply + sizeof(reply->magic),

-                   sizeof(*reply) - sizeof(reply->magic), errp);

+                   sizeof(*reply) - sizeof(reply->magic), "reply", errp);

     if (ret < 0) {

         return ret;

     }

@@ -1404,7 +1373,8 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc,

     assert(chunk->magic == NBD_STRUCTURED_REPLY_MAGIC);

     ret = nbd_read(ioc, (uint8_t *)chunk + sizeof(chunk->magic),

-                   sizeof(*chunk) - sizeof(chunk->magic), errp);

+                   sizeof(*chunk) - sizeof(chunk->magic), "structured chunk",

+                   errp);

     if (ret < 0) {

         return ret;

     }

diff --git a/nbd/common.c b/nbd/common.c

index 41f5ed8..cc8b278 100644

--- a/nbd/common.c

+++ b/nbd/common.c

@@ -31,7 +31,7 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)

     buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size));

     while (size > 0) {

         ssize_t count = MIN(65536, size);

-        ret = nbd_read(ioc, buffer, MIN(65536, size), errp);

+        ret = nbd_read(ioc, buffer, MIN(65536, size), NULL, errp);

         if (ret < 0) {

             goto cleanup;

diff --git a/nbd/server.c b/nbd/server.c

index cb0d563..838c150 100644

--- a/nbd/server.c

+++ b/nbd/server.c

@@ -438,8 +438,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client,

         error_setg(errp, "Bad length received");

         return -EINVAL;

     }

-    if (nbd_read(client->ioc, name, client->optlen, errp) < 0) {

-        error_prepend(errp, "read failed: ");

+    if (nbd_read(client->ioc, name, client->optlen, "export name", errp) < 0) {

         return -EIO;

     }

     name[client->optlen] = '\0';

@@ -1046,11 +1045,9 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,

         ...           Rest of request

     */

-    if (nbd_read(client->ioc, &flags, sizeof(flags), errp) < 0) {

-        error_prepend(errp, "read failed: ");

+    if (nbd_read32(client->ioc, &flags, "flags", errp) < 0) {

         return -EIO;

     }

-    flags = be32_to_cpu(flags);

     trace_nbd_negotiate_options_flags(flags);

     if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {

         fixedNewstyle = true;

@@ -1070,30 +1067,23 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,

         uint32_t option, length;

         uint64_t magic;

-        if (nbd_read(client->ioc, &magic, sizeof(magic), errp) < 0) {

-            error_prepend(errp, "read failed: ");

+        if (nbd_read64(client->ioc, &magic, "opts magic", errp) < 0) {

             return -EINVAL;

         }

-        magic = be64_to_cpu(magic);

         trace_nbd_negotiate_options_check_magic(magic);

         if (magic != NBD_OPTS_MAGIC) {

             error_setg(errp, "Bad magic received");

             return -EINVAL;

         }

-        if (nbd_read(client->ioc, &option,

-                     sizeof(option), errp) < 0) {

-            error_prepend(errp, "read failed: ");

+        if (nbd_read32(client->ioc, &option, "option", errp) < 0) {

             return -EINVAL;

         }

-        option = be32_to_cpu(option);

         client->opt = option;

-        if (nbd_read(client->ioc, &length, sizeof(length), errp) < 0) {

-            error_prepend(errp, "read failed: ");

+        if (nbd_read32(client->ioc, &length, "option length", errp) < 0) {

             return -EINVAL;

         }

-        length = be32_to_cpu(length);

         assert(!client->optlen);

         client->optlen = length;

@@ -1306,7 +1296,7 @@ static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request,

     uint32_t magic;

     int ret;

-    ret = nbd_read(ioc, buf, sizeof(buf), errp);

+    ret = nbd_read(ioc, buf, sizeof(buf), "request", errp);

     if (ret < 0) {

         return ret;

     }

@@ -2111,8 +2101,9 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,

         }

     }

     if (request->type == NBD_CMD_WRITE) {

-        if (nbd_read(client->ioc, req->data, request->len, errp) < 0) {

-            error_prepend(errp, "reading from socket failed: ");

+        if (nbd_read(client->ioc, req->data, request->len, "CMD_WRITE data",

+                     errp) < 0)

+        {

             return -EIO;

         }

         req->complete = true;

-- 

1.8.3.1