Skip to content

Commit

Permalink
nbd: read_sync and friends: return 0 on success
Browse files Browse the repository at this point in the history 
functions read_sync, drop_sync, write_sync, and also
nbd_negotiate_write, nbd_negotiate_read, nbd_negotiate_drop_sync
returns number of processed bytes. But what this number can be,
except requested number of bytes?

Actually, underlying nbd_wr_syncv function returns a value >= 0 and
!= requested_bytes only on eof on read operation. So, firstly, it is
impossible on write (let's add an assert) and on read it actually
means, that communication is broken (except nbd_receive_reply, see
below).

Most of callers operate like this:
   if (func(..., size) != size) {
       /* error path */
   }
, i.e.:
  1. They are not interested in partial success
  2. Extra duplications in code (especially bad are duplications of
     magic numbers)
  3. User doesn't see actual error message, as return code is lost.
     (this patch doesn't fix this point, but it makes fixing easier)

Several callers handles ret >= 0 and != requested-size separately, by
just returning EINVAL in this case. This patch makes read_sync and
friends return EINVAL in this case, so final behavior is the same.

And only one caller - nbd_receive_reply() does something not so
obvious. It returns EINVAL for ret > 0 and != requested-size, like
previous group, but for ret == 0 it returns 0. The only caller of
nbd_receive_reply() - nbd_read_reply_entry() handles ret == 0 in the
same way as ret < 0, so for now it doesn't matter. However, in
following commits error path handling will be improved and we'll need
to distinguish success from fail in this case too. So, this patch adds
separate helper for this case - read_sync_eof.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20170516094533.6160-3-vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
  • Loading branch information
@bonzini
Vladimir Sementsov-Ogievskiy authored and bonzini committed Jun 6, 2017
1 parent f250a42 commit f5d406f
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 93 deletions.
63 changes: 25 additions & 38 deletions nbd/client.c
View file
Expand Up @@ -86,24 +86,23 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
*/

/* Discard length bytes from channel. Return -errno on failure, or
* the amount of bytes consumed. */
static ssize_t drop_sync(QIOChannel *ioc, size_t size)
/* Discard length bytes from channel. Return -errno on failure and 0 on
* success*/
static int drop_sync(QIOChannel *ioc, size_t size)
{
ssize_t ret = 0;
char small[1024];
char *buffer;

buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size));
while (size > 0) {
ssize_t count = read_sync(ioc, buffer, MIN(65536, size));
ssize_t count = MIN(65536, size);
ret = read_sync(ioc, buffer, MIN(65536, size));

if (count <= 0) {
if (ret < 0) {
goto cleanup;
}
assert(count <= size);
size -= count;
ret += count;
}

cleanup:
Expand Down Expand Up @@ -136,12 +135,12 @@ static int nbd_send_option_request(QIOChannel *ioc, uint32_t opt,
stl_be_p(&req.option, opt);
stl_be_p(&req.length, len);

if (write_sync(ioc, &req, sizeof(req)) != sizeof(req)) {
if (write_sync(ioc, &req, sizeof(req)) < 0) {
error_setg(errp, "Failed to send option request header");
return -1;
}

if (len && write_sync(ioc, (char *) data, len) != len) {
if (len && write_sync(ioc, (char *) data, len) < 0) {
error_setg(errp, "Failed to send option request data");
return -1;
}
Expand Down Expand Up @@ -170,7 +169,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,
nbd_opt_reply *reply, Error **errp)
{
QEMU_BUILD_BUG_ON(sizeof(*reply) != 20);
if (read_sync(ioc, reply, sizeof(*reply)) != sizeof(*reply)) {
if (read_sync(ioc, reply, sizeof(*reply)) < 0) {
error_setg(errp, "failed to read option reply");
nbd_send_opt_abort(ioc);
return -1;
Expand Down Expand Up @@ -219,7 +218,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, nbd_opt_reply *reply,
goto cleanup;
}
msg = g_malloc(reply->length + 1);
if (read_sync(ioc, msg, reply->length) != reply->length) {
if (read_sync(ioc, msg, reply->length) < 0) {
error_setg(errp, "failed to read option error message");
goto cleanup;
}
Expand Down Expand Up @@ -321,7 +320,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
nbd_send_opt_abort(ioc);
return -1;
}
if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
if (read_sync(ioc, &namelen, sizeof(namelen)) < 0) {
error_setg(errp, "failed to read option name length");
nbd_send_opt_abort(ioc);
return -1;
Expand All @@ -334,7 +333,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
return -1;
}
if (namelen != strlen(want)) {
if (drop_sync(ioc, len) != len) {
if (drop_sync(ioc, len) < 0) {
error_setg(errp, "failed to skip export name with wrong length");
nbd_send_opt_abort(ioc);
return -1;
Expand All @@ -343,14 +342,14 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
}

assert(namelen < sizeof(name));
if (read_sync(ioc, name, namelen) != namelen) {
if (read_sync(ioc, name, namelen) < 0) {
error_setg(errp, "failed to read export name");
nbd_send_opt_abort(ioc);
return -1;
}
name[namelen] = '\0';
len -= namelen;
if (drop_sync(ioc, len) != len) {
if (drop_sync(ioc, len) < 0) {
error_setg(errp, "failed to read export description");
nbd_send_opt_abort(ioc);
return -1;
Expand Down Expand Up @@ -477,7 +476,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}

if (read_sync(ioc, buf, 8) != 8) {
if (read_sync(ioc, buf, 8) < 0) {
error_setg(errp, "Failed to read data");
goto fail;
}
Expand All @@ -503,7 +502,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}

if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
if (read_sync(ioc, &magic, sizeof(magic)) < 0) {
error_setg(errp, "Failed to read magic");
goto fail;
}
Expand All @@ -515,8 +514,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
uint16_t globalflags;
bool fixedNewStyle = false;

if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
sizeof(globalflags)) {
if (read_sync(ioc, &globalflags, sizeof(globalflags)) < 0) {
error_setg(errp, "Failed to read server flags");
goto fail;
}
Expand All @@ -534,8 +532,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
}
/* client requested flags */
clientflags = cpu_to_be32(clientflags);
if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
sizeof(clientflags)) {
if (write_sync(ioc, &clientflags, sizeof(clientflags)) < 0) {
error_setg(errp, "Failed to send clientflags field");
goto fail;
}
Expand Down Expand Up @@ -573,13 +570,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
}

/* Read the response */
if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
if (read_sync(ioc, &s, sizeof(s)) < 0) {
error_setg(errp, "Failed to read export length");
goto fail;
}
*size = be64_to_cpu(s);

if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
if (read_sync(ioc, flags, sizeof(*flags)) < 0) {
error_setg(errp, "Failed to read export flags");
goto fail;
}
Expand All @@ -596,14 +593,14 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}

if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
if (read_sync(ioc, &s, sizeof(s)) < 0) {
error_setg(errp, "Failed to read export length");
goto fail;
}
*size = be64_to_cpu(s);
TRACE("Size is %" PRIu64, *size);

if (read_sync(ioc, &oldflags, sizeof(oldflags)) != sizeof(oldflags)) {
if (read_sync(ioc, &oldflags, sizeof(oldflags)) < 0) {
error_setg(errp, "Failed to read export flags");
goto fail;
}
Expand All @@ -619,7 +616,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
}

TRACE("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags);
if (zeroes && drop_sync(ioc, 124) != 124) {
if (zeroes && drop_sync(ioc, 124) < 0) {
error_setg(errp, "Failed to read reserved block");
goto fail;
}
Expand Down Expand Up @@ -744,7 +741,6 @@ int nbd_disconnect(int fd)
ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
{
uint8_t buf[NBD_REQUEST_SIZE];
ssize_t ret;

TRACE("Sending request to server: "
"{ .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64
Expand All @@ -759,16 +755,7 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
stq_be_p(buf + 16, request->from);
stl_be_p(buf + 24, request->len);

ret = write_sync(ioc, buf, sizeof(buf));
if (ret < 0) {
return ret;
}

if (ret != sizeof(buf)) {
LOG("writing to socket failed");
return -EINVAL;
}
return 0;
return write_sync(ioc, buf, sizeof(buf));
}

ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
Expand All @@ -777,7 +764,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
uint32_t magic;
ssize_t ret;

ret = read_sync(ioc, buf, sizeof(buf));
ret = read_sync_eof(ioc, buf, sizeof(buf));
if (ret <= 0) {
return ret;
}
Expand Down
34 changes: 30 additions & 4 deletions nbd/nbd-internal.h
View file
Expand Up @@ -94,7 +94,13 @@
#define NBD_ENOSPC 28
#define NBD_ESHUTDOWN 108

static inline ssize_t read_sync(QIOChannel *ioc, void *buffer, size_t size)
/* read_sync_eof
* Tries to read @size bytes from @ioc. Returns number of bytes actually read.
* May return a value >= 0 and < size only on EOF, i.e. when iteratively called
* qio_channel_readv() returns 0. So, there are no needs to call read_sync_eof
* iteratively.
*/
static inline ssize_t read_sync_eof(QIOChannel *ioc, void *buffer, size_t size)
{
struct iovec iov = { .iov_base = buffer, .iov_len = size };
/* Sockets are kept in blocking mode in the negotiation phase. After
Expand All @@ -105,12 +111,32 @@ static inline ssize_t read_sync(QIOChannel *ioc, void *buffer, size_t size)
return nbd_wr_syncv(ioc, &iov, 1, size, true);
}

static inline ssize_t write_sync(QIOChannel *ioc, const void *buffer,
size_t size)
/* read_sync
* Reads @size bytes from @ioc. Returns 0 on success.
*/
static inline int read_sync(QIOChannel *ioc, void *buffer, size_t size)
{
ssize_t ret = read_sync_eof(ioc, buffer, size);

if (ret >= 0 && ret != size) {
ret = -EINVAL;
}

return ret < 0 ? ret : 0;
}

/* write_sync
* Writes @size bytes to @ioc. Returns 0 on success.
*/
static inline int write_sync(QIOChannel *ioc, const void *buffer, size_t size)
{
struct iovec iov = { .iov_base = (void *) buffer, .iov_len = size };

return nbd_wr_syncv(ioc, &iov, 1, size, false);
ssize_t ret = nbd_wr_syncv(ioc, &iov, 1, size, false);

assert(ret < 0 || ret == size);

return ret < 0 ? ret : 0;
}

struct NBDTLSHandshakeData {
Expand Down

0 comments on commit f5d406f

Please sign in to comment.