From nobody Fri Nov 7 08:04:25 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=fail(p=none dis=none) header.from=virtuozzo.com Return-Path: Received: from lists.gnu.org (209.51.188.17 [209.51.188.17]) by mx.zohomail.com with SMTPS id 1547648741352940.6740806446106; Wed, 16 Jan 2019 06:25:41 -0800 (PST) Received: from localhost ([127.0.0.1]:36513 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gjm8M-0002iI-Uf for importer@patchew.org; Wed, 16 Jan 2019 09:25:34 -0500 Received: from eggs.gnu.org ([209.51.188.92]:53175) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gjm6t-000212-OH for qemu-devel@nongnu.org; Wed, 16 Jan 2019 09:24:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gjm6r-0001me-Mo for qemu-devel@nongnu.org; Wed, 16 Jan 2019 09:24:03 -0500 Received: from relay.sw.ru ([185.231.240.75]:51016) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gjm6r-0001he-6j; Wed, 16 Jan 2019 09:24:01 -0500 Received: from [10.28.8.145] (helo=kvm.sw.ru) by relay.sw.ru with esmtp (Exim 4.91) (envelope-from ) id 1gjm6n-00026k-3B; Wed, 16 Jan 2019 17:23:57 +0300 From: Vladimir Sementsov-Ogievskiy To: qemu-devel@nongnu.org, qemu-block@nongnu.org Date: Wed, 16 Jan 2019 17:23:56 +0300 Message-Id: <20190116142356.31515-1-vsementsov@virtuozzo.com> X-Mailer: git-send-email 2.18.0 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 185.231.240.75 Subject: [Qemu-devel] [RFC] nbd: generalize usage of nbd_read X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kwolf@redhat.com, mreitz@redhat.com Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" 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 Possible TODO: make a helper to read sized variable buffers, like uint32_t len @len bytes buffer follows Signed-off-by: Vladimir Sementsov-Ogievskiy --- Hi Eric! I have an idea of several helpers around nbd_read, what do you think about it? It now based on your "nbd: add qemu-nbd --list", and if we like it, I'll fix iotests appropriately (if they fail, but I think, they should) and rebase onto final version of your series. Based-on:<20190112175812.27068-1-eblake@redhat.com> include/block/nbd.h | 33 ++++++++++++++++- block/nbd-client.c | 5 +-- nbd/client.c | 89 ++++++++++++++------------------------------- nbd/common.c | 2 +- nbd/server.c | 27 +++++--------- 5 files changed, 71 insertions(+), 85 deletions(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index 80ee3cc997..f3d3ffc749 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" =20 /* Handshake phase structs - this struct is passed on the wire */ =20 @@ -336,11 +337,39 @@ 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 =3D qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO := 0; + + if (ret < 0) { + if (desc) { + error_prepend(errp, "Failed to read %s: ", desc); + } else { + error_prepend(errp, "Failed to read from socket: "); + } + return -1; + } + + return 0; } =20 +#define DEF_NBD_READ(bits) \ +static inline int nbd_read ## bits(QIOChannel *ioc, uint ## bits ## _t *va= l, \ + const char *desc, Error **errp) = \ +{ = \ + if (nbd_read(ioc, val, sizeof(*val), desc, errp) < 0) { = \ + return -1; = \ + } = \ + *val =3D be ## bits ## _to_cpu(*val); = \ + return 0; = \ +} + +DEF_NBD_READ(16) +DEF_NBD_READ(32) +DEF_NBD_READ(64) + +#undef DEF_NBD_READ + static inline bool nbd_reply_is_simple(NBDReply *reply) { return reply->magic =3D=3D NBD_SIMPLE_REPLY_MAGIC; diff --git a/block/nbd-client.c b/block/nbd-client.c index 813539676d..5c97052608 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -337,10 +337,9 @@ static int nbd_co_receive_offset_data_payload(NBDClien= tSession *s, return -EINVAL; } =20 - 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); =20 data_size =3D chunk->length - sizeof(offset); assert(data_size); @@ -387,7 +386,7 @@ static coroutine_fn int nbd_co_receive_structured_paylo= ad( } =20 *payload =3D g_new(char, len); - ret =3D nbd_read(s->ioc, *payload, len, errp); + ret =3D nbd_read(s->ioc, *payload, len, "structured payload", errp); if (ret < 0) { g_free(*payload); *payload =3D NULL; diff --git a/nbd/client.c b/nbd/client.c index 64f3e45edd..bd62efe1de 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -113,8 +113,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, ui= nt32_t opt, NBDOptionReply *reply, Error **errp) { QEMU_BUILD_BUG_ON(sizeof(*reply) !=3D 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,10 +165,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOp= tionReply *reply, goto cleanup; } msg =3D g_malloc(reply->length + 1); - if (nbd_read(ioc, msg, reply->length, errp) < 0) { - error_prepend(errp, "failed to read option error %" PRIu32 - " (%s) message: ", - reply->type, nbd_rep_lookup(reply->type)); + if (nbd_read(ioc, msg, reply->length, "option error", errp) < 0) { goto cleanup; } msg[reply->length] =3D '\0'; @@ -284,12 +280,10 @@ static int nbd_receive_list(QIOChannel *ioc, char **n= ame, 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 =3D be32_to_cpu(namelen); len -=3D sizeof(namelen); if (len < namelen) { error_setg(errp, "incorrect option name length"); @@ -298,8 +292,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **nam= e, char **description, } =20 local_name =3D 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 +300,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **nam= e, char **description, len -=3D namelen; if (len) { local_desc =3D 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; } @@ -330,7 +322,6 @@ static int nbd_receive_list(QIOChannel *ioc, char **nam= e, char **description, return ret; } =20 - /* * Returns -1 if NBD_OPT_GO proves the export @info->name cannot be * used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and @@ -406,13 +397,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 -=3D sizeof(type); - type =3D be16_to_cpu(type); switch (type) { case NBD_INFO_EXPORT: if (len !=3D sizeof(info->size) + sizeof(info->flags)) { @@ -421,18 +410,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 =3D 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 =3D be16_to_cpu(info->flags); trace_nbd_receive_negotiate_size_flags(info->size, info->flags= ); break; =20 @@ -443,27 +428,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 siz= e: "); + if (nbd_read32(ioc, &info->min_block, "info minimum block size= ", + errp) < 0) { nbd_send_opt_abort(ioc); return -1; } - info->min_block =3D 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 si= ze", + errp) < 0) + { nbd_send_opt_abort(ioc); return -1; } - info->opt_block =3D 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 @@ -471,13 +452,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 siz= e: "); + if (nbd_read32(ioc, &info->max_block, "info maximum block size= ", + errp) < 0) + { nbd_send_opt_abort(ioc); return -1; } - info->max_block =3D 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); @@ -730,14 +710,14 @@ static int nbd_receive_one_meta_context(QIOChannel *i= oc, return -1; } =20 - 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 =3D be32_to_cpu(local_id); =20 reply.length -=3D sizeof(local_id); local_name =3D 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; } @@ -893,11 +873,9 @@ static int nbd_start_negotiate(QIOChannel *ioc, QCrypt= oTLSCreds *tlscreds, return -EINVAL; } =20 - 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 =3D be64_to_cpu(magic); trace_nbd_receive_negotiate_magic(magic); =20 if (magic !=3D NBD_INIT_MAGIC) { @@ -905,11 +883,9 @@ static int nbd_start_negotiate(QIOChannel *ioc, QCrypt= oTLSCreds *tlscreds, return -EINVAL; } =20 - 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 =3D be64_to_cpu(magic); trace_nbd_receive_negotiate_magic(magic); =20 if (magic =3D=3D NBD_OPTS_MAGIC) { @@ -917,11 +893,9 @@ static int nbd_start_negotiate(QIOChannel *ioc, QCrypt= oTLSCreds *tlscreds, uint16_t globalflags; bool fixedNewStyle =3D false; =20 - 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 =3D be16_to_cpu(globalflags); trace_nbd_receive_negotiate_server_flags(globalflags); if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) { fixedNewStyle =3D true; @@ -989,17 +963,13 @@ static int nbd_negotiate_finish_oldstyle(QIOChannel *= ioc, NBDExportInfo *info, { uint32_t oldflags; =20 - 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 =3D be64_to_cpu(info->size); =20 - 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 =3D be32_to_cpu(oldflags); if (oldflags & ~0xffff) { error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags); return -EINVAL; @@ -1076,17 +1046,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoT= LSCreds *tlscreds, } =20 /* 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 =3D be64_to_cpu(info->size); =20 - 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 =3D be16_to_cpu(info->flags); break; case 0: /* oldstyle, parse length and flags */ if (*info->name) { @@ -1374,7 +1340,7 @@ static int nbd_receive_simple_reply(QIOChannel *ioc, = NBDSimpleReply *reply, assert(reply->magic =3D=3D NBD_SIMPLE_REPLY_MAGIC); =20 ret =3D 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; } @@ -1399,7 +1365,8 @@ static int nbd_receive_structured_reply_chunk(QIOChan= nel *ioc, assert(chunk->magic =3D=3D NBD_STRUCTURED_REPLY_MAGIC); =20 ret =3D nbd_read(ioc, (uint8_t *)chunk + sizeof(chunk->magic), - sizeof(*chunk) - sizeof(chunk->magic), errp); + sizeof(*chunk) - sizeof(chunk->magic), "structured chun= k", + errp); if (ret < 0) { return ret; } diff --git a/nbd/common.c b/nbd/common.c index 41f5ed8d9f..cc8b278e54 100644 --- a/nbd/common.c +++ b/nbd/common.c @@ -31,7 +31,7 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp) buffer =3D sizeof(small) >=3D size ? small : g_malloc(MIN(65536, size)= ); while (size > 0) { ssize_t count =3D MIN(65536, size); - ret =3D nbd_read(ioc, buffer, MIN(65536, size), errp); + ret =3D nbd_read(ioc, buffer, MIN(65536, size), NULL, errp); =20 if (ret < 0) { goto cleanup; diff --git a/nbd/server.c b/nbd/server.c index 15357d40fd..97a38de40c 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] =3D '\0'; @@ -1046,11 +1045,9 @@ static int nbd_negotiate_options(NBDClient *client, = uint16_t myflags, ... Rest of request */ =20 - 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 =3D be32_to_cpu(flags); trace_nbd_negotiate_options_flags(flags); if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) { fixedNewstyle =3D true; @@ -1070,30 +1067,23 @@ static int nbd_negotiate_options(NBDClient *client,= uint16_t myflags, uint32_t option, length; uint64_t magic; =20 - 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 =3D be64_to_cpu(magic); trace_nbd_negotiate_options_check_magic(magic); if (magic !=3D NBD_OPTS_MAGIC) { error_setg(errp, "Bad magic received"); return -EINVAL; } =20 - 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 =3D be32_to_cpu(option); client->opt =3D option; =20 - 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 =3D be32_to_cpu(length); assert(!client->optlen); client->optlen =3D length; =20 @@ -1306,7 +1296,7 @@ static int nbd_receive_request(QIOChannel *ioc, NBDRe= quest *request, uint32_t magic; int ret; =20 - ret =3D nbd_read(ioc, buf, sizeof(buf), errp); + ret =3D nbd_read(ioc, buf, sizeof(buf), "request", errp); if (ret < 0) { return ret; } @@ -2110,8 +2100,9 @@ static int nbd_co_receive_request(NBDRequestData *req= , NBDRequest *request, } } if (request->type =3D=3D 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 =3D true; --=20 2.18.0