1 From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
2 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
3 Date: Tue, 16 May 2017 12:45:30 +0300
4 Subject: [PATCH] nbd: read_sync and friends: return 0 on success
6 functions read_sync, drop_sync, write_sync, and also
7 nbd_negotiate_write, nbd_negotiate_read, nbd_negotiate_drop_sync
8 returns number of processed bytes. But what this number can be,
9 except requested number of bytes?
11 Actually, underlying nbd_wr_syncv function returns a value >= 0 and
12 != requested_bytes only on eof on read operation. So, firstly, it is
13 impossible on write (let's add an assert) and on read it actually
14 means, that communication is broken (except nbd_receive_reply, see
17 Most of callers operate like this:
18 if (func(..., size) != size) {
22 1. They are not interested in partial success
23 2. Extra duplications in code (especially bad are duplications of
25 3. User doesn't see actual error message, as return code is lost.
26 (this patch doesn't fix this point, but it makes fixing easier)
28 Several callers handles ret >= 0 and != requested-size separately, by
29 just returning EINVAL in this case. This patch makes read_sync and
30 friends return EINVAL in this case, so final behavior is the same.
32 And only one caller - nbd_receive_reply() does something not so
33 obvious. It returns EINVAL for ret > 0 and != requested-size, like
34 previous group, but for ret == 0 it returns 0. The only caller of
35 nbd_receive_reply() - nbd_read_reply_entry() handles ret == 0 in the
36 same way as ret < 0, so for now it doesn't matter. However, in
37 following commits error path handling will be improved and we'll need
38 to distinguish success from fail in this case too. So, this patch adds
39 separate helper for this case - read_sync_eof.
41 Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
42 Message-Id: <20170516094533.6160-3-vsementsov@virtuozzo.com>
43 Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
45 nbd/client.c | 63 ++++++++++++++++------------------------
46 nbd/nbd-internal.h | 34 +++++++++++++++++++---
47 nbd/server.c | 84 +++++++++++++++++++++---------------------------------
48 3 files changed, 88 insertions(+), 93 deletions(-)
50 diff --git a/nbd/client.c b/nbd/client.c
51 index a58fb02cb4..6b74a628f1 100644
54 @@ -86,9 +86,9 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
58 -/* Discard length bytes from channel. Return -errno on failure, or
59 - * the amount of bytes consumed. */
60 -static ssize_t drop_sync(QIOChannel *ioc, size_t size)
61 +/* Discard length bytes from channel. Return -errno on failure and 0 on
63 +static int drop_sync(QIOChannel *ioc, size_t size)
67 @@ -96,14 +96,13 @@ static ssize_t drop_sync(QIOChannel *ioc, size_t size)
69 buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size));
71 - ssize_t count = read_sync(ioc, buffer, MIN(65536, size));
72 + ssize_t count = MIN(65536, size);
73 + ret = read_sync(ioc, buffer, MIN(65536, size));
79 - assert(count <= size);
85 @@ -136,12 +135,12 @@ static int nbd_send_option_request(QIOChannel *ioc, uint32_t opt,
86 stl_be_p(&req.option, opt);
87 stl_be_p(&req.length, len);
89 - if (write_sync(ioc, &req, sizeof(req)) != sizeof(req)) {
90 + if (write_sync(ioc, &req, sizeof(req)) < 0) {
91 error_setg(errp, "Failed to send option request header");
95 - if (len && write_sync(ioc, (char *) data, len) != len) {
96 + if (len && write_sync(ioc, (char *) data, len) < 0) {
97 error_setg(errp, "Failed to send option request data");
100 @@ -170,7 +169,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,
101 nbd_opt_reply *reply, Error **errp)
103 QEMU_BUILD_BUG_ON(sizeof(*reply) != 20);
104 - if (read_sync(ioc, reply, sizeof(*reply)) != sizeof(*reply)) {
105 + if (read_sync(ioc, reply, sizeof(*reply)) < 0) {
106 error_setg(errp, "failed to read option reply");
107 nbd_send_opt_abort(ioc);
109 @@ -219,7 +218,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, nbd_opt_reply *reply,
112 msg = g_malloc(reply->length + 1);
113 - if (read_sync(ioc, msg, reply->length) != reply->length) {
114 + if (read_sync(ioc, msg, reply->length) < 0) {
115 error_setg(errp, "failed to read option error message");
118 @@ -321,7 +320,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
119 nbd_send_opt_abort(ioc);
122 - if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
123 + if (read_sync(ioc, &namelen, sizeof(namelen)) < 0) {
124 error_setg(errp, "failed to read option name length");
125 nbd_send_opt_abort(ioc);
127 @@ -334,7 +333,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
130 if (namelen != strlen(want)) {
131 - if (drop_sync(ioc, len) != len) {
132 + if (drop_sync(ioc, len) < 0) {
133 error_setg(errp, "failed to skip export name with wrong length");
134 nbd_send_opt_abort(ioc);
136 @@ -343,14 +342,14 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
139 assert(namelen < sizeof(name));
140 - if (read_sync(ioc, name, namelen) != namelen) {
141 + if (read_sync(ioc, name, namelen) < 0) {
142 error_setg(errp, "failed to read export name");
143 nbd_send_opt_abort(ioc);
146 name[namelen] = '\0';
148 - if (drop_sync(ioc, len) != len) {
149 + if (drop_sync(ioc, len) < 0) {
150 error_setg(errp, "failed to read export description");
151 nbd_send_opt_abort(ioc);
153 @@ -477,7 +476,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
157 - if (read_sync(ioc, buf, 8) != 8) {
158 + if (read_sync(ioc, buf, 8) < 0) {
159 error_setg(errp, "Failed to read data");
162 @@ -503,7 +502,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
166 - if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
167 + if (read_sync(ioc, &magic, sizeof(magic)) < 0) {
168 error_setg(errp, "Failed to read magic");
171 @@ -515,8 +514,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
172 uint16_t globalflags;
173 bool fixedNewStyle = false;
175 - if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
176 - sizeof(globalflags)) {
177 + if (read_sync(ioc, &globalflags, sizeof(globalflags)) < 0) {
178 error_setg(errp, "Failed to read server flags");
181 @@ -534,8 +532,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
183 /* client requested flags */
184 clientflags = cpu_to_be32(clientflags);
185 - if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
186 - sizeof(clientflags)) {
187 + if (write_sync(ioc, &clientflags, sizeof(clientflags)) < 0) {
188 error_setg(errp, "Failed to send clientflags field");
191 @@ -573,13 +570,13 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
194 /* Read the response */
195 - if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
196 + if (read_sync(ioc, &s, sizeof(s)) < 0) {
197 error_setg(errp, "Failed to read export length");
200 *size = be64_to_cpu(s);
202 - if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
203 + if (read_sync(ioc, flags, sizeof(*flags)) < 0) {
204 error_setg(errp, "Failed to read export flags");
207 @@ -596,14 +593,14 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
211 - if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
212 + if (read_sync(ioc, &s, sizeof(s)) < 0) {
213 error_setg(errp, "Failed to read export length");
216 *size = be64_to_cpu(s);
217 TRACE("Size is %" PRIu64, *size);
219 - if (read_sync(ioc, &oldflags, sizeof(oldflags)) != sizeof(oldflags)) {
220 + if (read_sync(ioc, &oldflags, sizeof(oldflags)) < 0) {
221 error_setg(errp, "Failed to read export flags");
224 @@ -619,7 +616,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
227 TRACE("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags);
228 - if (zeroes && drop_sync(ioc, 124) != 124) {
229 + if (zeroes && drop_sync(ioc, 124) < 0) {
230 error_setg(errp, "Failed to read reserved block");
233 @@ -744,7 +741,6 @@ int nbd_disconnect(int fd)
234 ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
236 uint8_t buf[NBD_REQUEST_SIZE];
239 TRACE("Sending request to server: "
240 "{ .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64
241 @@ -759,16 +755,7 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
242 stq_be_p(buf + 16, request->from);
243 stl_be_p(buf + 24, request->len);
245 - ret = write_sync(ioc, buf, sizeof(buf));
250 - if (ret != sizeof(buf)) {
251 - LOG("writing to socket failed");
255 + return write_sync(ioc, buf, sizeof(buf));
258 ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
259 @@ -777,7 +764,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply)
263 - ret = read_sync(ioc, buf, sizeof(buf));
264 + ret = read_sync_eof(ioc, buf, sizeof(buf));
268 diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
269 index f43d990a05..e6bbc7c4b4 100644
270 --- a/nbd/nbd-internal.h
271 +++ b/nbd/nbd-internal.h
273 #define NBD_ENOSPC 28
274 #define NBD_ESHUTDOWN 108
276 -static inline ssize_t read_sync(QIOChannel *ioc, void *buffer, size_t size)
278 + * Tries to read @size bytes from @ioc. Returns number of bytes actually read.
279 + * May return a value >= 0 and < size only on EOF, i.e. when iteratively called
280 + * qio_channel_readv() returns 0. So, there are no needs to call read_sync_eof
283 +static inline ssize_t read_sync_eof(QIOChannel *ioc, void *buffer, size_t size)
285 struct iovec iov = { .iov_base = buffer, .iov_len = size };
286 /* Sockets are kept in blocking mode in the negotiation phase. After
287 @@ -105,12 +111,32 @@ static inline ssize_t read_sync(QIOChannel *ioc, void *buffer, size_t size)
288 return nbd_wr_syncv(ioc, &iov, 1, size, true);
291 -static inline ssize_t write_sync(QIOChannel *ioc, const void *buffer,
294 + * Reads @size bytes from @ioc. Returns 0 on success.
296 +static inline int read_sync(QIOChannel *ioc, void *buffer, size_t size)
298 + ssize_t ret = read_sync_eof(ioc, buffer, size);
300 + if (ret >= 0 && ret != size) {
304 + return ret < 0 ? ret : 0;
308 + * Writes @size bytes to @ioc. Returns 0 on success.
310 +static inline int write_sync(QIOChannel *ioc, const void *buffer, size_t size)
312 struct iovec iov = { .iov_base = (void *) buffer, .iov_len = size };
314 - return nbd_wr_syncv(ioc, &iov, 1, size, false);
315 + ssize_t ret = nbd_wr_syncv(ioc, &iov, 1, size, false);
317 + assert(ret < 0 || ret == size);
319 + return ret < 0 ? ret : 0;
322 struct NBDTLSHandshakeData {
323 diff --git a/nbd/server.c b/nbd/server.c
324 index 4d6da8ac06..c2a5909ad6 100644
327 @@ -112,7 +112,7 @@ static gboolean nbd_negotiate_continue(QIOChannel *ioc,
331 -static ssize_t nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
332 +static int nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
336 @@ -130,8 +130,7 @@ static ssize_t nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
340 -static ssize_t nbd_negotiate_write(QIOChannel *ioc, const void *buffer,
342 +static int nbd_negotiate_write(QIOChannel *ioc, const void *buffer, size_t size)
346 @@ -148,24 +147,24 @@ static ssize_t nbd_negotiate_write(QIOChannel *ioc, const void *buffer,
350 -static ssize_t nbd_negotiate_drop_sync(QIOChannel *ioc, size_t size)
351 +static int nbd_negotiate_drop_sync(QIOChannel *ioc, size_t size)
353 - ssize_t ret, dropped = size;
355 uint8_t *buffer = g_malloc(MIN(65536, size));
358 - ret = nbd_negotiate_read(ioc, buffer, MIN(65536, size));
359 + size_t count = MIN(65536, size);
360 + ret = nbd_negotiate_read(ioc, buffer, count);
366 - assert(ret <= size);
376 /* Basic flow for negotiation
377 @@ -206,22 +205,22 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
380 magic = cpu_to_be64(NBD_REP_MAGIC);
381 - if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
382 + if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) < 0) {
383 LOG("write failed (rep magic)");
386 opt = cpu_to_be32(opt);
387 - if (nbd_negotiate_write(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
388 + if (nbd_negotiate_write(ioc, &opt, sizeof(opt)) < 0) {
389 LOG("write failed (rep opt)");
392 type = cpu_to_be32(type);
393 - if (nbd_negotiate_write(ioc, &type, sizeof(type)) != sizeof(type)) {
394 + if (nbd_negotiate_write(ioc, &type, sizeof(type)) < 0) {
395 LOG("write failed (rep type)");
398 len = cpu_to_be32(len);
399 - if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) {
400 + if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) {
401 LOG("write failed (rep data length)");
404 @@ -256,7 +255,7 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
408 - if (nbd_negotiate_write(ioc, msg, len) != len) {
409 + if (nbd_negotiate_write(ioc, msg, len) < 0) {
410 LOG("write failed (error message)");
413 @@ -287,15 +286,15 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
416 len = cpu_to_be32(name_len);
417 - if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) {
418 + if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) {
419 LOG("write failed (name length)");
422 - if (nbd_negotiate_write(ioc, name, name_len) != name_len) {
423 + if (nbd_negotiate_write(ioc, name, name_len) < 0) {
424 LOG("write failed (name buffer)");
427 - if (nbd_negotiate_write(ioc, desc, desc_len) != desc_len) {
428 + if (nbd_negotiate_write(ioc, desc, desc_len) < 0) {
429 LOG("write failed (description buffer)");
432 @@ -309,7 +308,7 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
436 - if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
437 + if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
440 return nbd_negotiate_send_rep_err(client->ioc,
441 @@ -340,7 +339,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
442 LOG("Bad length received");
445 - if (nbd_negotiate_read(client->ioc, name, length) != length) {
446 + if (nbd_negotiate_read(client->ioc, name, length) < 0) {
450 @@ -373,7 +372,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
451 TRACE("Setting up TLS");
454 - if (nbd_negotiate_drop_sync(ioc, length) != length) {
455 + if (nbd_negotiate_drop_sync(ioc, length) < 0) {
458 nbd_negotiate_send_rep_err(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS,
459 @@ -437,8 +436,7 @@ static int nbd_negotiate_options(NBDClient *client)
463 - if (nbd_negotiate_read(client->ioc, &flags, sizeof(flags)) !=
465 + if (nbd_negotiate_read(client->ioc, &flags, sizeof(flags)) < 0) {
469 @@ -464,8 +462,7 @@ static int nbd_negotiate_options(NBDClient *client)
470 uint32_t clientflags, length;
473 - if (nbd_negotiate_read(client->ioc, &magic, sizeof(magic)) !=
475 + if (nbd_negotiate_read(client->ioc, &magic, sizeof(magic)) < 0) {
479 @@ -476,14 +473,14 @@ static int nbd_negotiate_options(NBDClient *client)
482 if (nbd_negotiate_read(client->ioc, &clientflags,
483 - sizeof(clientflags)) != sizeof(clientflags)) {
484 + sizeof(clientflags)) < 0)
489 clientflags = be32_to_cpu(clientflags);
491 - if (nbd_negotiate_read(client->ioc, &length, sizeof(length)) !=
493 + if (nbd_negotiate_read(client->ioc, &length, sizeof(length)) < 0) {
497 @@ -519,7 +516,7 @@ static int nbd_negotiate_options(NBDClient *client)
501 - if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
502 + if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
505 ret = nbd_negotiate_send_rep_err(client->ioc,
506 @@ -557,7 +554,7 @@ static int nbd_negotiate_options(NBDClient *client)
507 return nbd_negotiate_handle_export_name(client, length);
509 case NBD_OPT_STARTTLS:
510 - if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
511 + if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
514 if (client->tlscreds) {
515 @@ -576,7 +573,7 @@ static int nbd_negotiate_options(NBDClient *client)
519 - if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
520 + if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
523 ret = nbd_negotiate_send_rep_err(client->ioc,
524 @@ -665,12 +662,12 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
525 TRACE("TLS cannot be enabled with oldstyle protocol");
528 - if (nbd_negotiate_write(client->ioc, buf, sizeof(buf)) != sizeof(buf)) {
529 + if (nbd_negotiate_write(client->ioc, buf, sizeof(buf)) < 0) {
534 - if (nbd_negotiate_write(client->ioc, buf, 18) != 18) {
535 + if (nbd_negotiate_write(client->ioc, buf, 18) < 0) {
539 @@ -685,7 +682,7 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
540 stq_be_p(buf + 18, client->exp->size);
541 stw_be_p(buf + 26, client->exp->nbdflags | myflags);
542 len = client->no_zeroes ? 10 : sizeof(buf) - 18;
543 - if (nbd_negotiate_write(client->ioc, buf + 18, len) != len) {
544 + if (nbd_negotiate_write(client->ioc, buf + 18, len) < 0) {
548 @@ -708,11 +705,6 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
552 - if (ret != sizeof(buf)) {
553 - LOG("read failed");
558 [ 0 .. 3] magic (NBD_REQUEST_MAGIC)
559 [ 4 .. 5] flags (NBD_CMD_FLAG_FUA, ...)
560 @@ -743,7 +735,6 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
561 static ssize_t nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
563 uint8_t buf[NBD_REPLY_SIZE];
566 reply->error = system_errno_to_nbd_errno(reply->error);
568 @@ -760,16 +751,7 @@ static ssize_t nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
569 stl_be_p(buf + 4, reply->error);
570 stq_be_p(buf + 8, reply->handle);
572 - ret = write_sync(ioc, buf, sizeof(buf));
577 - if (ret != sizeof(buf)) {
578 - LOG("writing to socket failed");
582 + return write_sync(ioc, buf, sizeof(buf));
585 #define MAX_NBD_REQUESTS 16
586 @@ -1073,7 +1055,7 @@ static ssize_t nbd_co_send_reply(NBDRequestData *req, NBDReply *reply,
587 rc = nbd_send_reply(client->ioc, reply);
589 ret = write_sync(client->ioc, req->data, len);
595 @@ -1147,7 +1129,7 @@ static ssize_t nbd_co_receive_request(NBDRequestData *req,
596 if (request->type == NBD_CMD_WRITE) {
597 TRACE("Reading %" PRIu32 " byte(s)", request->len);
599 - if (read_sync(client->ioc, req->data, request->len) != request->len) {
600 + if (read_sync(client->ioc, req->data, request->len) < 0) {
601 LOG("reading from socket failed");