* [Qemu-devel] [PATCH v2 01/12] nbd: rename read_sync and friends
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 02/12] nbd: make nbd_drop public Vladimir Sementsov-Ogievskiy
` (11 subsequent siblings)
12 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
Rename
nbd_wr_syncv -> nbd_rwv
read_sync -> nbd_read
read_sync_eof -> nbd_read_eof
write_sync -> nbd_write
drop_sync -> nbd_drop
1. nbd_ prefix
read_sync and write_sync are already shared, so it is good to have a
namespace prefix. drop_sync will be shared, and read_sync_eof is
related to read_sync, so let's rename them all.
2. _sync suffix
_sync is related to the fact that nbd_wr_syncv doesn't return if a
write to socket returns EAGAIN. The first implementation of
nbd_wr_syncv (was wr_sync in 7a5ca8648b) just loops while getting
EAGAIN, the current implementation yields in this case.
Why we want to get rid of it:
- it is normal for r/w functions to be synchronous, so having an
additional suffix for it looks redundant (contrariwise, we have
_aio suffix for async functions)
- _sync suffix in block layer is used when function does flush (so
using it for other thing is confusing a bit)
- keep function names short after adding nbd_ prefix
3. for nbd_wr_syncv let's use more common notation 'rw'
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/nbd-client.c | 8 ++++----
include/block/nbd.h | 8 ++------
nbd/client.c | 42 +++++++++++++++++++++---------------------
nbd/common.c | 8 ++------
nbd/nbd-internal.h | 26 +++++++++++++-------------
nbd/server.c | 12 ++++++------
6 files changed, 48 insertions(+), 56 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 09d955bc4d..3e080ca7f0 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -140,8 +140,8 @@ static int nbd_co_send_request(BlockDriverState *bs,
qio_channel_set_cork(s->ioc, true);
rc = nbd_send_request(s->ioc, request);
if (rc >= 0) {
- ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov, request->len,
- false, NULL);
+ ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
+ NULL);
if (ret != request->len) {
rc = -EIO;
}
@@ -169,8 +169,8 @@ static void nbd_co_receive_reply(NBDClientSession *s,
reply->error = EIO;
} else {
if (qiov && reply->error == 0) {
- ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov, request->len,
- true, NULL);
+ ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, true,
+ NULL);
if (ret != request->len) {
reply->error = EIO;
}
diff --git a/include/block/nbd.h b/include/block/nbd.h
index e0c64e4981..0a5ecd0e5c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -123,12 +123,8 @@ enum {
* aren't overflowing some other buffer. */
#define NBD_MAX_NAME_SIZE 256
-ssize_t nbd_wr_syncv(QIOChannel *ioc,
- struct iovec *iov,
- size_t niov,
- size_t length,
- bool do_read,
- Error **errp);
+ssize_t nbd_rwv(QIOChannel *ioc, struct iovec *iov, size_t niov, size_t length,
+ bool do_read, Error **errp);
int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
QCryptoTLSCreds *tlscreds, const char *hostname,
QIOChannel **outioc,
diff --git a/nbd/client.c b/nbd/client.c
index f9e1d75be4..08050c39b3 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -88,7 +88,7 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
/* Discard length bytes from channel. Return -errno on failure and 0 on
* success*/
-static int drop_sync(QIOChannel *ioc, size_t size, Error **errp)
+static int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
{
ssize_t ret = 0;
char small[1024];
@@ -97,7 +97,7 @@ static int drop_sync(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 = read_sync(ioc, buffer, MIN(65536, size), errp);
+ ret = nbd_read(ioc, buffer, MIN(65536, size), errp);
if (ret < 0) {
goto cleanup;
@@ -135,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), errp) < 0) {
+ if (nbd_write(ioc, &req, sizeof(req), errp) < 0) {
error_prepend(errp, "Failed to send option request header");
return -1;
}
- if (len && write_sync(ioc, (char *) data, len, errp) < 0) {
+ if (len && nbd_write(ioc, (char *) data, len, errp) < 0) {
error_prepend(errp, "Failed to send option request data");
return -1;
}
@@ -169,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), errp) < 0) {
+ if (nbd_read(ioc, reply, sizeof(*reply), errp) < 0) {
error_prepend(errp, "failed to read option reply");
nbd_send_opt_abort(ioc);
return -1;
@@ -218,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, errp) < 0) {
+ if (nbd_read(ioc, msg, reply->length, errp) < 0) {
error_prepend(errp, "failed to read option error message");
goto cleanup;
}
@@ -320,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), errp) < 0) {
+ if (nbd_read(ioc, &namelen, sizeof(namelen), errp) < 0) {
error_prepend(errp, "failed to read option name length");
nbd_send_opt_abort(ioc);
return -1;
@@ -333,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, errp) < 0) {
+ if (nbd_drop(ioc, len, errp) < 0) {
error_prepend(errp, "failed to skip export name with wrong length");
nbd_send_opt_abort(ioc);
return -1;
@@ -342,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, errp) < 0) {
+ if (nbd_read(ioc, name, namelen, errp) < 0) {
error_prepend(errp, "failed to read export name");
nbd_send_opt_abort(ioc);
return -1;
}
name[namelen] = '\0';
len -= namelen;
- if (drop_sync(ioc, len, errp) < 0) {
+ if (nbd_drop(ioc, len, errp) < 0) {
error_prepend(errp, "failed to read export description");
nbd_send_opt_abort(ioc);
return -1;
@@ -476,7 +476,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}
- if (read_sync(ioc, buf, 8, errp) < 0) {
+ if (nbd_read(ioc, buf, 8, errp) < 0) {
error_prepend(errp, "Failed to read data");
goto fail;
}
@@ -502,7 +502,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}
- if (read_sync(ioc, &magic, sizeof(magic), errp) < 0) {
+ if (nbd_read(ioc, &magic, sizeof(magic), errp) < 0) {
error_prepend(errp, "Failed to read magic");
goto fail;
}
@@ -514,7 +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), errp) < 0) {
+ if (nbd_read(ioc, &globalflags, sizeof(globalflags), errp) < 0) {
error_prepend(errp, "Failed to read server flags");
goto fail;
}
@@ -532,7 +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), errp) < 0) {
+ if (nbd_write(ioc, &clientflags, sizeof(clientflags), errp) < 0) {
error_prepend(errp, "Failed to send clientflags field");
goto fail;
}
@@ -570,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), errp) < 0) {
+ if (nbd_read(ioc, &s, sizeof(s), errp) < 0) {
error_prepend(errp, "Failed to read export length");
goto fail;
}
*size = be64_to_cpu(s);
- if (read_sync(ioc, flags, sizeof(*flags), errp) < 0) {
+ if (nbd_read(ioc, flags, sizeof(*flags), errp) < 0) {
error_prepend(errp, "Failed to read export flags");
goto fail;
}
@@ -593,14 +593,14 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
goto fail;
}
- if (read_sync(ioc, &s, sizeof(s), errp) < 0) {
+ if (nbd_read(ioc, &s, sizeof(s), errp) < 0) {
error_prepend(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), errp) < 0) {
+ if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) {
error_prepend(errp, "Failed to read export flags");
goto fail;
}
@@ -616,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, errp) < 0) {
+ if (zeroes && nbd_drop(ioc, 124, errp) < 0) {
error_prepend(errp, "Failed to read reserved block");
goto fail;
}
@@ -759,7 +759,7 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
stq_be_p(buf + 16, request->from);
stl_be_p(buf + 24, request->len);
- return write_sync(ioc, buf, sizeof(buf), NULL);
+ return nbd_write(ioc, buf, sizeof(buf), NULL);
}
ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
@@ -768,7 +768,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
uint32_t magic;
ssize_t ret;
- ret = read_sync_eof(ioc, buf, sizeof(buf), errp);
+ ret = nbd_read_eof(ioc, buf, sizeof(buf), errp);
if (ret <= 0) {
return ret;
}
diff --git a/nbd/common.c b/nbd/common.c
index bd81637ab9..d6b719ddaa 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -24,12 +24,8 @@
* The function may be called from coroutine or from non-coroutine context.
* When called from non-coroutine context @ioc must be in blocking mode.
*/
-ssize_t nbd_wr_syncv(QIOChannel *ioc,
- struct iovec *iov,
- size_t niov,
- size_t length,
- bool do_read,
- Error **errp)
+ssize_t nbd_rwv(QIOChannel *ioc, struct iovec *iov, size_t niov, size_t length,
+ bool do_read, Error **errp)
{
ssize_t done = 0;
struct iovec *local_iov = g_new(struct iovec, niov);
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index d6071640a0..753cb9d9c5 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -94,14 +94,14 @@
#define NBD_ENOSPC 28
#define NBD_ESHUTDOWN 108
-/* read_sync_eof
+/* nbd_read_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
+ * qio_channel_readv() returns 0. So, there is no need to call nbd_read_eof
* iteratively.
*/
-static inline ssize_t read_sync_eof(QIOChannel *ioc, void *buffer, size_t size,
- Error **errp)
+static inline ssize_t nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,
+ Error **errp)
{
struct iovec iov = { .iov_base = buffer, .iov_len = size };
/* Sockets are kept in blocking mode in the negotiation phase. After
@@ -109,16 +109,16 @@ static inline ssize_t read_sync_eof(QIOChannel *ioc, void *buffer, size_t size,
* our request/reply. Synchronization is done with recv_coroutine, so
* that this is coroutine-safe.
*/
- return nbd_wr_syncv(ioc, &iov, 1, size, true, errp);
+ return nbd_rwv(ioc, &iov, 1, size, true, errp);
}
-/* read_sync
+/* nbd_read
* Reads @size bytes from @ioc. Returns 0 on success.
*/
-static inline int read_sync(QIOChannel *ioc, void *buffer, size_t size,
- Error **errp)
+static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
+ Error **errp)
{
- ssize_t ret = read_sync_eof(ioc, buffer, size, errp);
+ ssize_t ret = nbd_read_eof(ioc, buffer, size, errp);
if (ret >= 0 && ret != size) {
ret = -EINVAL;
@@ -128,15 +128,15 @@ static inline int read_sync(QIOChannel *ioc, void *buffer, size_t size,
return ret < 0 ? ret : 0;
}
-/* write_sync
+/* nbd_write
* Writes @size bytes to @ioc. Returns 0 on success.
*/
-static inline int write_sync(QIOChannel *ioc, const void *buffer, size_t size,
- Error **errp)
+static inline int nbd_write(QIOChannel *ioc, const void *buffer, size_t size,
+ Error **errp)
{
struct iovec iov = { .iov_base = (void *) buffer, .iov_len = size };
- ssize_t ret = nbd_wr_syncv(ioc, &iov, 1, size, false, errp);
+ ssize_t ret = nbd_rwv(ioc, &iov, 1, size, false, errp);
assert(ret < 0 || ret == size);
diff --git a/nbd/server.c b/nbd/server.c
index ee59e5d234..abaf9fb890 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -124,7 +124,7 @@ static int nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
nbd_negotiate_continue,
qemu_coroutine_self(),
NULL);
- ret = read_sync(ioc, buffer, size, NULL);
+ ret = nbd_read(ioc, buffer, size, NULL);
g_source_remove(watch);
return ret;
@@ -142,7 +142,7 @@ static int nbd_negotiate_write(QIOChannel *ioc, const void *buffer, size_t size)
nbd_negotiate_continue,
qemu_coroutine_self(),
NULL);
- ret = write_sync(ioc, buffer, size, NULL);
+ ret = nbd_write(ioc, buffer, size, NULL);
g_source_remove(watch);
return ret;
}
@@ -694,7 +694,7 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
uint32_t magic;
ssize_t ret;
- ret = read_sync(ioc, buf, sizeof(buf), NULL);
+ ret = nbd_read(ioc, buf, sizeof(buf), NULL);
if (ret < 0) {
return ret;
}
@@ -745,7 +745,7 @@ static ssize_t nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
stl_be_p(buf + 4, reply->error);
stq_be_p(buf + 8, reply->handle);
- return write_sync(ioc, buf, sizeof(buf), NULL);
+ return nbd_write(ioc, buf, sizeof(buf), NULL);
}
#define MAX_NBD_REQUESTS 16
@@ -1048,7 +1048,7 @@ static ssize_t nbd_co_send_reply(NBDRequestData *req, NBDReply *reply,
qio_channel_set_cork(client->ioc, true);
rc = nbd_send_reply(client->ioc, reply);
if (rc >= 0) {
- ret = write_sync(client->ioc, req->data, len, NULL);
+ ret = nbd_write(client->ioc, req->data, len, NULL);
if (ret < 0) {
rc = -EIO;
}
@@ -1123,7 +1123,7 @@ static ssize_t nbd_co_receive_request(NBDRequestData *req,
if (request->type == NBD_CMD_WRITE) {
TRACE("Reading %" PRIu32 " byte(s)", request->len);
- if (read_sync(client->ioc, req->data, request->len, NULL) < 0) {
+ if (nbd_read(client->ioc, req->data, request->len, NULL) < 0) {
LOG("reading from socket failed");
rc = -EIO;
goto out;
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 02/12] nbd: make nbd_drop public
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 01/12] nbd: rename read_sync and friends Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 03/12] nbd/server: get rid of nbd_negotiate_read and friends Vladimir Sementsov-Ogievskiy
` (10 subsequent siblings)
12 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
Following commit will reuse it for nbd server too.
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
nbd/client.c | 26 --------------------------
nbd/common.c | 26 ++++++++++++++++++++++++++
nbd/nbd-internal.h | 2 ++
3 files changed, 28 insertions(+), 26 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 08050c39b3..215857f65d 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -86,32 +86,6 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
*/
-/* Discard length bytes from channel. Return -errno on failure and 0 on
- * success*/
-static int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
-{
- 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 = MIN(65536, size);
- ret = nbd_read(ioc, buffer, MIN(65536, size), errp);
-
- if (ret < 0) {
- goto cleanup;
- }
- size -= count;
- }
-
- cleanup:
- if (buffer != small) {
- g_free(buffer);
- }
- return ret;
-}
-
/* Send an option request.
*
* The request is for option @opt, with @data containing @len bytes of
diff --git a/nbd/common.c b/nbd/common.c
index d6b719ddaa..6b5c1b7b02 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -65,6 +65,32 @@ ssize_t nbd_rwv(QIOChannel *ioc, struct iovec *iov, size_t niov, size_t length,
return done;
}
+/* Discard length bytes from channel. Return -errno on failure and 0 on
+ * success */
+int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
+{
+ 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 = MIN(65536, size);
+ ret = nbd_read(ioc, buffer, MIN(65536, size), errp);
+
+ if (ret < 0) {
+ goto cleanup;
+ }
+ size -= count;
+ }
+
+ cleanup:
+ if (buffer != small) {
+ g_free(buffer);
+ }
+ return ret;
+}
+
void nbd_tls_handshake(QIOTask *task,
void *opaque)
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 753cb9d9c5..39bfed177c 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -153,4 +153,6 @@ struct NBDTLSHandshakeData {
void nbd_tls_handshake(QIOTask *task,
void *opaque);
+int nbd_drop(QIOChannel *ioc, size_t size, Error **errp);
+
#endif
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 03/12] nbd/server: get rid of nbd_negotiate_read and friends
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 01/12] nbd: rename read_sync and friends Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 02/12] nbd: make nbd_drop public Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-07-18 12:04 ` Eric Blake
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 04/12] nbd/server: get rid of ssize_t Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
12 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
Functions nbd_negotiate_{read,write,drop_sync} were introduced in
1a6245a5b, when nbd_rwv (was nbd_wr_sync) was working through
qemu_co_sendv_recvv (the path is nbd_wr_sync -> qemu_co_{recv/send} ->
qemu_co_send_recv -> qemu_co_sendv_recvv), which just yields, without
setting any handlers. But starting from ff82911cd nbd_rwv (was
nbd_wr_syncv) works through qio_channel_yield() which sets handlers, so
watchers are redundant in nbd_negotiate_{read,write,drop_sync}, then,
let's just use nbd_{read,write,drop} functions.
Functions nbd_{read,write,drop} has errp parameter, which is unused in
this patch. This will be fixed later.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 107 ++++++++++++-----------------------------------------------
1 file changed, 22 insertions(+), 85 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index abaf9fb890..b830cd23d3 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -104,69 +104,6 @@ struct NBDClient {
static void nbd_client_receive_next_request(NBDClient *client);
-static gboolean nbd_negotiate_continue(QIOChannel *ioc,
- GIOCondition condition,
- void *opaque)
-{
- qemu_coroutine_enter(opaque);
- return TRUE;
-}
-
-static int nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
-{
- ssize_t ret;
- guint watch;
-
- assert(qemu_in_coroutine());
- /* Negotiation are always in main loop. */
- watch = qio_channel_add_watch(ioc,
- G_IO_IN,
- nbd_negotiate_continue,
- qemu_coroutine_self(),
- NULL);
- ret = nbd_read(ioc, buffer, size, NULL);
- g_source_remove(watch);
- return ret;
-
-}
-
-static int nbd_negotiate_write(QIOChannel *ioc, const void *buffer, size_t size)
-{
- ssize_t ret;
- guint watch;
-
- assert(qemu_in_coroutine());
- /* Negotiation are always in main loop. */
- watch = qio_channel_add_watch(ioc,
- G_IO_OUT,
- nbd_negotiate_continue,
- qemu_coroutine_self(),
- NULL);
- ret = nbd_write(ioc, buffer, size, NULL);
- g_source_remove(watch);
- return ret;
-}
-
-static int nbd_negotiate_drop_sync(QIOChannel *ioc, size_t size)
-{
- ssize_t ret;
- uint8_t *buffer = g_malloc(MIN(65536, size));
-
- while (size > 0) {
- size_t count = MIN(65536, size);
- ret = nbd_negotiate_read(ioc, buffer, count);
- if (ret < 0) {
- g_free(buffer);
- return ret;
- }
-
- size -= count;
- }
-
- g_free(buffer);
- return 0;
-}
-
/* Basic flow for negotiation
Server Client
@@ -205,22 +142,22 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
type, opt, len);
magic = cpu_to_be64(NBD_REP_MAGIC);
- if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) < 0) {
+ if (nbd_write(ioc, &magic, sizeof(magic), NULL) < 0) {
LOG("write failed (rep magic)");
return -EINVAL;
}
opt = cpu_to_be32(opt);
- if (nbd_negotiate_write(ioc, &opt, sizeof(opt)) < 0) {
+ if (nbd_write(ioc, &opt, sizeof(opt), NULL) < 0) {
LOG("write failed (rep opt)");
return -EINVAL;
}
type = cpu_to_be32(type);
- if (nbd_negotiate_write(ioc, &type, sizeof(type)) < 0) {
+ if (nbd_write(ioc, &type, sizeof(type), NULL) < 0) {
LOG("write failed (rep type)");
return -EINVAL;
}
len = cpu_to_be32(len);
- if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) {
+ if (nbd_write(ioc, &len, sizeof(len), NULL) < 0) {
LOG("write failed (rep data length)");
return -EINVAL;
}
@@ -255,7 +192,7 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
if (ret < 0) {
goto out;
}
- if (nbd_negotiate_write(ioc, msg, len) < 0) {
+ if (nbd_write(ioc, msg, len, NULL) < 0) {
LOG("write failed (error message)");
ret = -EIO;
} else {
@@ -286,15 +223,15 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
}
len = cpu_to_be32(name_len);
- if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) {
+ if (nbd_write(ioc, &len, sizeof(len), NULL) < 0) {
LOG("write failed (name length)");
return -EINVAL;
}
- if (nbd_negotiate_write(ioc, name, name_len) < 0) {
+ if (nbd_write(ioc, name, name_len, NULL) < 0) {
LOG("write failed (name buffer)");
return -EINVAL;
}
- if (nbd_negotiate_write(ioc, desc, desc_len) < 0) {
+ if (nbd_write(ioc, desc, desc_len, NULL) < 0) {
LOG("write failed (description buffer)");
return -EINVAL;
}
@@ -308,7 +245,7 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
NBDExport *exp;
if (length) {
- if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+ if (nbd_drop(client->ioc, length, NULL) < 0) {
return -EIO;
}
return nbd_negotiate_send_rep_err(client->ioc,
@@ -339,7 +276,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
LOG("Bad length received");
goto fail;
}
- if (nbd_negotiate_read(client->ioc, name, length) < 0) {
+ if (nbd_read(client->ioc, name, length, NULL) < 0) {
LOG("read failed");
goto fail;
}
@@ -372,7 +309,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
TRACE("Setting up TLS");
ioc = client->ioc;
if (length) {
- if (nbd_negotiate_drop_sync(ioc, length) < 0) {
+ if (nbd_drop(ioc, length, NULL) < 0) {
return NULL;
}
nbd_negotiate_send_rep_err(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS,
@@ -436,7 +373,7 @@ static int nbd_negotiate_options(NBDClient *client)
... Rest of request
*/
- if (nbd_negotiate_read(client->ioc, &flags, sizeof(flags)) < 0) {
+ if (nbd_read(client->ioc, &flags, sizeof(flags), NULL) < 0) {
LOG("read failed");
return -EIO;
}
@@ -462,7 +399,7 @@ static int nbd_negotiate_options(NBDClient *client)
uint32_t clientflags, length;
uint64_t magic;
- if (nbd_negotiate_read(client->ioc, &magic, sizeof(magic)) < 0) {
+ if (nbd_read(client->ioc, &magic, sizeof(magic), NULL) < 0) {
LOG("read failed");
return -EINVAL;
}
@@ -472,15 +409,15 @@ static int nbd_negotiate_options(NBDClient *client)
return -EINVAL;
}
- if (nbd_negotiate_read(client->ioc, &clientflags,
- sizeof(clientflags)) < 0)
+ if (nbd_read(client->ioc, &clientflags,
+ sizeof(clientflags), NULL) < 0)
{
LOG("read failed");
return -EINVAL;
}
clientflags = be32_to_cpu(clientflags);
- if (nbd_negotiate_read(client->ioc, &length, sizeof(length)) < 0) {
+ if (nbd_read(client->ioc, &length, sizeof(length), NULL) < 0) {
LOG("read failed");
return -EINVAL;
}
@@ -510,7 +447,7 @@ static int nbd_negotiate_options(NBDClient *client)
return -EINVAL;
default:
- if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+ if (nbd_drop(client->ioc, length, NULL) < 0) {
return -EIO;
}
ret = nbd_negotiate_send_rep_err(client->ioc,
@@ -548,7 +485,7 @@ static int nbd_negotiate_options(NBDClient *client)
return nbd_negotiate_handle_export_name(client, length);
case NBD_OPT_STARTTLS:
- if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+ if (nbd_drop(client->ioc, length, NULL) < 0) {
return -EIO;
}
if (client->tlscreds) {
@@ -567,7 +504,7 @@ static int nbd_negotiate_options(NBDClient *client)
}
break;
default:
- if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+ if (nbd_drop(client->ioc, length, NULL) < 0) {
return -EIO;
}
ret = nbd_negotiate_send_rep_err(client->ioc,
@@ -656,12 +593,12 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
TRACE("TLS cannot be enabled with oldstyle protocol");
goto fail;
}
- if (nbd_negotiate_write(client->ioc, buf, sizeof(buf)) < 0) {
+ if (nbd_write(client->ioc, buf, sizeof(buf), NULL) < 0) {
LOG("write failed");
goto fail;
}
} else {
- if (nbd_negotiate_write(client->ioc, buf, 18) < 0) {
+ if (nbd_write(client->ioc, buf, 18, NULL) < 0) {
LOG("write failed");
goto fail;
}
@@ -676,7 +613,7 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
stq_be_p(buf + 18, client->exp->size);
stw_be_p(buf + 26, client->exp->nbdflags | myflags);
len = client->no_zeroes ? 10 : sizeof(buf) - 18;
- if (nbd_negotiate_write(client->ioc, buf + 18, len) < 0) {
+ if (nbd_write(client->ioc, buf + 18, len, NULL) < 0) {
LOG("write failed");
goto fail;
}
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2 03/12] nbd/server: get rid of nbd_negotiate_read and friends
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 03/12] nbd/server: get rid of nbd_negotiate_read and friends Vladimir Sementsov-Ogievskiy
@ 2017-07-18 12:04 ` Eric Blake
0 siblings, 0 replies; 16+ messages in thread
From: Eric Blake @ 2017-07-18 12:04 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel; +Cc: pbonzini, den
[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]
On 06/02/2017 10:01 AM, Vladimir Sementsov-Ogievskiy wrote:
> Functions nbd_negotiate_{read,write,drop_sync} were introduced in
> 1a6245a5b, when nbd_rwv (was nbd_wr_sync) was working through
> qemu_co_sendv_recvv (the path is nbd_wr_sync -> qemu_co_{recv/send} ->
> qemu_co_send_recv -> qemu_co_sendv_recvv), which just yields, without
> setting any handlers. But starting from ff82911cd nbd_rwv (was
> nbd_wr_syncv) works through qio_channel_yield() which sets handlers, so
> watchers are redundant in nbd_negotiate_{read,write,drop_sync}, then,
> let's just use nbd_{read,write,drop} functions.
>
> Functions nbd_{read,write,drop} has errp parameter, which is unused in
> this patch. This will be fixed later.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> nbd/server.c | 107 ++++++++++++-----------------------------------------------
> 1 file changed, 22 insertions(+), 85 deletions(-)
I did not realize it at the time, but this patch plugs a
denial-of-service security hole against malicious clients that were able
to trigger an assertion failure in the server by sending garbage during
negotiation; which was a regression introduced in the mentioned commit
ff82911cd. This has now been assigned the identifier CVE-2017-7539
The fact that we have now had 4 CVEs against qemu's NBD implementation
in the last year means we are not doing a very good job of unit testing
either the server or the client against a malicious partner; I'm still
trying to figure out ways that we can improve our testsuite coverage
(testing that a sane client can still connect happens during
qemu-iotests, but most of our CVEs have happened due to poor reactions
to out-of-spec clients).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v2 04/12] nbd/server: get rid of ssize_t
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 03/12] nbd/server: get rid of nbd_negotiate_read and friends Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 05/12] nbd/server: refactor nbd_co_send_reply Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
12 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
Now nbd_read and friends return int, so get rid of ssize_t.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index b830cd23d3..8985df86b4 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -625,11 +625,11 @@ fail:
return rc;
}
-static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
+static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
{
uint8_t buf[NBD_REQUEST_SIZE];
uint32_t magic;
- ssize_t ret;
+ int ret;
ret = nbd_read(ioc, buf, sizeof(buf), NULL);
if (ret < 0) {
@@ -663,7 +663,7 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
return 0;
}
-static ssize_t nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
+static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
{
uint8_t buf[NBD_REPLY_SIZE];
@@ -969,11 +969,10 @@ void nbd_export_close_all(void)
}
}
-static ssize_t nbd_co_send_reply(NBDRequestData *req, NBDReply *reply,
- int len)
+static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
{
NBDClient *client = req->client;
- ssize_t rc, ret;
+ int rc, ret;
g_assert(qemu_in_coroutine());
qemu_co_mutex_lock(&client->send_lock);
@@ -1003,11 +1002,10 @@ static ssize_t nbd_co_send_reply(NBDRequestData *req, NBDReply *reply,
* and any other negative value to report an error to the client
* (although the caller may still need to disconnect after reporting
* the error). */
-static ssize_t nbd_co_receive_request(NBDRequestData *req,
- NBDRequest *request)
+static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
{
NBDClient *client = req->client;
- ssize_t rc;
+ int rc;
g_assert(qemu_in_coroutine());
assert(client->recv_coroutine == qemu_coroutine_self());
@@ -1105,7 +1103,7 @@ static coroutine_fn void nbd_trip(void *opaque)
NBDRequestData *req;
NBDRequest request = { 0 }; /* GCC thinks it can be used uninitialized */
NBDReply reply;
- ssize_t ret;
+ int ret;
int flags;
TRACE("Reading request.");
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 05/12] nbd/server: refactor nbd_co_send_reply
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 04/12] nbd/server: get rid of ssize_t Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 06/12] nbd/server: get rid of EAGAIN dead code Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
12 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
As nbd_write never returns value > 0, we can get rid of extra ret.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 8985df86b4..dd6b241761 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -972,7 +972,7 @@ void nbd_export_close_all(void)
static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
{
NBDClient *client = req->client;
- int rc, ret;
+ int rc;
g_assert(qemu_in_coroutine());
qemu_co_mutex_lock(&client->send_lock);
@@ -983,9 +983,9 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
} else {
qio_channel_set_cork(client->ioc, true);
rc = nbd_send_reply(client->ioc, reply);
- if (rc >= 0) {
- ret = nbd_write(client->ioc, req->data, len, NULL);
- if (ret < 0) {
+ if (rc == 0) {
+ rc = nbd_write(client->ioc, req->data, len, NULL);
+ if (rc < 0) {
rc = -EIO;
}
}
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 06/12] nbd/server: get rid of EAGAIN dead code
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 05/12] nbd/server: refactor nbd_co_send_reply Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 07/12] nbd/server: refactor nbd_co_receive_request Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
12 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
For now nbd_read never returns EAGAIN. So, don't handle it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index dd6b241761..b0bb263596 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -997,11 +997,12 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
return rc;
}
-/* Collect a client request. Return 0 if request looks valid, -EAGAIN
- * to keep trying the collection, -EIO to drop connection right away,
- * and any other negative value to report an error to the client
- * (although the caller may still need to disconnect after reporting
- * the error). */
+/* nbd_co_receive_request
+ * Collect a client request. Return 0 if request looks valid, -EIO to drop
+ * connection right away, and any other negative value to report an error to
+ * the client (although the caller may still need to disconnect after reporting
+ * the error).
+ */
static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
{
NBDClient *client = req->client;
@@ -1011,9 +1012,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
assert(client->recv_coroutine == qemu_coroutine_self());
rc = nbd_receive_request(client->ioc, request);
if (rc < 0) {
- if (rc != -EAGAIN) {
- rc = -EIO;
- }
+ rc = -EIO;
goto out;
}
@@ -1114,9 +1113,6 @@ static coroutine_fn void nbd_trip(void *opaque)
req = nbd_request_get(client);
ret = nbd_co_receive_request(req, &request);
- if (ret == -EAGAIN) {
- goto done;
- }
if (ret == -EIO) {
goto out;
}
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 07/12] nbd/server: refactor nbd_co_receive_request
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 06/12] nbd/server: get rid of EAGAIN dead code Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 08/12] nbd/server: remove NBDClientNewData Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
12 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
Move function tail, about receiving next request out of the function.
Error path is simplified and nbd_co_receive_request becomes more
corresponding to its name.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 41 +++++++++++++----------------------------
1 file changed, 13 insertions(+), 28 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index b0bb263596..8ac095d6bc 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1006,14 +1006,11 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
{
NBDClient *client = req->client;
- int rc;
g_assert(qemu_in_coroutine());
assert(client->recv_coroutine == qemu_coroutine_self());
- rc = nbd_receive_request(client->ioc, request);
- if (rc < 0) {
- rc = -EIO;
- goto out;
+ if (nbd_receive_request(client->ioc, request) < 0) {
+ return -EIO;
}
TRACE("Decoding type");
@@ -1027,8 +1024,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
/* Special case: we're going to disconnect without a reply,
* whether or not flags, from, or len are bogus */
TRACE("Request type is DISCONNECT");
- rc = -EIO;
- goto out;
+ return -EIO;
}
/* Check for sanity in the parameters, part 1. Defer as many
@@ -1036,22 +1032,19 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
* payload, so we can try and keep the connection alive. */
if ((request->from + request->len) < request->from) {
LOG("integer overflow detected, you're probably being attacked");
- rc = -EINVAL;
- goto out;
+ return -EINVAL;
}
if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) {
if (request->len > NBD_MAX_BUFFER_SIZE) {
LOG("len (%" PRIu32" ) is larger than max len (%u)",
request->len, NBD_MAX_BUFFER_SIZE);
- rc = -EINVAL;
- goto out;
+ return -EINVAL;
}
req->data = blk_try_blockalign(client->exp->blk, request->len);
if (req->data == NULL) {
- rc = -ENOMEM;
- goto out;
+ return -ENOMEM;
}
}
if (request->type == NBD_CMD_WRITE) {
@@ -1059,8 +1052,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
if (nbd_read(client->ioc, req->data, request->len, NULL) < 0) {
LOG("reading from socket failed");
- rc = -EIO;
- goto out;
+ return -EIO;
}
req->complete = true;
}
@@ -1070,28 +1062,19 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
", Size: %" PRIu64, request->from, request->len,
(uint64_t)client->exp->size);
- rc = request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
- goto out;
+ return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
}
if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
LOG("unsupported flags (got 0x%x)", request->flags);
- rc = -EINVAL;
- goto out;
+ return -EINVAL;
}
if (request->type != NBD_CMD_WRITE_ZEROES &&
(request->flags & NBD_CMD_FLAG_NO_HOLE)) {
LOG("unexpected flags (got 0x%x)", request->flags);
- rc = -EINVAL;
- goto out;
+ return -EINVAL;
}
- rc = 0;
-
-out:
- client->recv_coroutine = NULL;
- nbd_client_receive_next_request(client);
-
- return rc;
+ return 0;
}
/* Owns a reference to the NBDClient passed as opaque. */
@@ -1113,6 +1096,8 @@ static coroutine_fn void nbd_trip(void *opaque)
req = nbd_request_get(client);
ret = nbd_co_receive_request(req, &request);
+ client->recv_coroutine = NULL;
+ nbd_client_receive_next_request(client);
if (ret == -EIO) {
goto out;
}
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 08/12] nbd/server: remove NBDClientNewData
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 07/12] nbd/server: refactor nbd_co_receive_request Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 09/12] nbd/server: nbd_negotiate: fix error path Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
12 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
"co" field of NBDClientNewData has never been used, all the way back to
its declaration in commit 1a6245a5. So let's just use client pointer
instead of extra structure.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 25 +++++++------------------
1 file changed, 7 insertions(+), 18 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 8ac095d6bc..d376563527 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -535,14 +535,8 @@ static int nbd_negotiate_options(NBDClient *client)
}
}
-typedef struct {
- NBDClient *client;
- Coroutine *co;
-} NBDClientNewData;
-
-static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
+static coroutine_fn int nbd_negotiate(NBDClient *client)
{
- NBDClient *client = data->client;
char buf[8 + 8 + 8 + 128];
int rc;
const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
@@ -1268,16 +1262,15 @@ static void nbd_client_receive_next_request(NBDClient *client)
static coroutine_fn void nbd_co_client_start(void *opaque)
{
- NBDClientNewData *data = opaque;
- NBDClient *client = data->client;
+ NBDClient *client = opaque;
NBDExport *exp = client->exp;
if (exp) {
nbd_export_get(exp);
}
- if (nbd_negotiate(data)) {
+ if (nbd_negotiate(client)) {
client_close(client);
- goto out;
+ return;
}
qemu_co_mutex_init(&client->send_lock);
@@ -1286,9 +1279,6 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
}
nbd_client_receive_next_request(client);
-
-out:
- g_free(data);
}
void nbd_client_new(NBDExport *exp,
@@ -1298,7 +1288,7 @@ void nbd_client_new(NBDExport *exp,
void (*close_fn)(NBDClient *))
{
NBDClient *client;
- NBDClientNewData *data = g_new(NBDClientNewData, 1);
+ Coroutine *co;
client = g_malloc0(sizeof(NBDClient));
client->refcount = 1;
@@ -1314,7 +1304,6 @@ void nbd_client_new(NBDExport *exp,
object_ref(OBJECT(client->ioc));
client->close = close_fn;
- data->client = client;
- data->co = qemu_coroutine_create(nbd_co_client_start, data);
- qemu_coroutine_enter(data->co);
+ co = qemu_coroutine_create(nbd_co_client_start, client);
+ qemu_coroutine_enter(co);
}
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 09/12] nbd/server: nbd_negotiate: fix error path
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 08/12] nbd/server: remove NBDClientNewData Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 10/12] nbd/server: get rid of fail: return rc Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
12 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
Current code will return 0 on this nbd_write fail, as rc is 0
after successful nbd_negotiate_options. Fix this.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/nbd/server.c b/nbd/server.c
index d376563527..ec163ad829 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -607,7 +607,8 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
stq_be_p(buf + 18, client->exp->size);
stw_be_p(buf + 26, client->exp->nbdflags | myflags);
len = client->no_zeroes ? 10 : sizeof(buf) - 18;
- if (nbd_write(client->ioc, buf + 18, len, NULL) < 0) {
+ rc = nbd_write(client->ioc, buf + 18, len, NULL);
+ if (rc < 0) {
LOG("write failed");
goto fail;
}
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 10/12] nbd/server: get rid of fail: return rc
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 09/12] nbd/server: nbd_negotiate: fix error path Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 11/12] nbd/server: rename rc to ret Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
12 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
"goto fail" error handling scheme is not needed for just returning
error code. Better is return it immediately.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index ec163ad829..2f1c5b0a5b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -265,7 +265,6 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
{
- int rc = -EINVAL;
char name[NBD_MAX_NAME_SIZE + 1];
/* Client sends:
@@ -274,11 +273,11 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
TRACE("Checking length");
if (length >= sizeof(name)) {
LOG("Bad length received");
- goto fail;
+ return -EINVAL;
}
if (nbd_read(client->ioc, name, length, NULL) < 0) {
LOG("read failed");
- goto fail;
+ return -EINVAL;
}
name[length] = '\0';
@@ -287,14 +286,13 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
client->exp = nbd_export_find(name);
if (!client->exp) {
LOG("export not found");
- goto fail;
+ return -EINVAL;
}
QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
nbd_export_get(client->exp);
- rc = 0;
-fail:
- return rc;
+
+ return 0;
}
/* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
@@ -564,7 +562,6 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
*/
qio_channel_set_blocking(client->ioc, false, NULL);
- rc = -EINVAL;
TRACE("Beginning negotiation.");
memset(buf, 0, sizeof(buf));
@@ -585,21 +582,21 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
if (oldStyle) {
if (client->tlscreds) {
TRACE("TLS cannot be enabled with oldstyle protocol");
- goto fail;
+ return -EINVAL;
}
if (nbd_write(client->ioc, buf, sizeof(buf), NULL) < 0) {
LOG("write failed");
- goto fail;
+ return -EINVAL;
}
} else {
if (nbd_write(client->ioc, buf, 18, NULL) < 0) {
LOG("write failed");
- goto fail;
+ return -EINVAL;
}
rc = nbd_negotiate_options(client);
if (rc != 0) {
LOG("option negotiation failed");
- goto fail;
+ return rc;
}
TRACE("advertising size %" PRIu64 " and flags %x",
@@ -610,14 +607,13 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
rc = nbd_write(client->ioc, buf + 18, len, NULL);
if (rc < 0) {
LOG("write failed");
- goto fail;
+ return rc;
}
}
TRACE("Negotiation succeeded.");
- rc = 0;
-fail:
- return rc;
+
+ return 0;
}
static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 11/12] nbd/server: rename rc to ret
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 10/12] nbd/server: get rid of fail: return rc Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 12/12] nbd/server: refactor nbd_trip Vladimir Sementsov-Ogievskiy
2017-06-13 14:10 ` [Qemu-devel] ping Re: [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
12 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
For consistency use 'ret' name for saving return code everywhere
in the file.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 2f1c5b0a5b..209a3d4e1e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -211,15 +211,15 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
uint32_t len;
const char *name = exp->name ? exp->name : "";
const char *desc = exp->description ? exp->description : "";
- int rc;
+ int ret;
TRACE("Advertising export name '%s' description '%s'", name, desc);
name_len = strlen(name);
desc_len = strlen(desc);
len = name_len + desc_len + sizeof(len);
- rc = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len);
- if (rc < 0) {
- return rc;
+ ret = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len);
+ if (ret < 0) {
+ return ret;
}
len = cpu_to_be32(name_len);
@@ -536,7 +536,7 @@ static int nbd_negotiate_options(NBDClient *client)
static coroutine_fn int nbd_negotiate(NBDClient *client)
{
char buf[8 + 8 + 8 + 128];
- int rc;
+ int ret;
const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
NBD_FLAG_SEND_WRITE_ZEROES);
@@ -593,10 +593,10 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
LOG("write failed");
return -EINVAL;
}
- rc = nbd_negotiate_options(client);
- if (rc != 0) {
+ ret = nbd_negotiate_options(client);
+ if (ret != 0) {
LOG("option negotiation failed");
- return rc;
+ return ret;
}
TRACE("advertising size %" PRIu64 " and flags %x",
@@ -604,10 +604,10 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
stq_be_p(buf + 18, client->exp->size);
stw_be_p(buf + 26, client->exp->nbdflags | myflags);
len = client->no_zeroes ? 10 : sizeof(buf) - 18;
- rc = nbd_write(client->ioc, buf + 18, len, NULL);
- if (rc < 0) {
+ ret = nbd_write(client->ioc, buf + 18, len, NULL);
+ if (ret < 0) {
LOG("write failed");
- return rc;
+ return ret;
}
}
@@ -963,21 +963,21 @@ void nbd_export_close_all(void)
static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
{
NBDClient *client = req->client;
- int rc;
+ int ret;
g_assert(qemu_in_coroutine());
qemu_co_mutex_lock(&client->send_lock);
client->send_coroutine = qemu_coroutine_self();
if (!len) {
- rc = nbd_send_reply(client->ioc, reply);
+ ret = nbd_send_reply(client->ioc, reply);
} else {
qio_channel_set_cork(client->ioc, true);
- rc = nbd_send_reply(client->ioc, reply);
- if (rc == 0) {
- rc = nbd_write(client->ioc, req->data, len, NULL);
- if (rc < 0) {
- rc = -EIO;
+ ret = nbd_send_reply(client->ioc, reply);
+ if (ret == 0) {
+ ret = nbd_write(client->ioc, req->data, len, NULL);
+ if (ret < 0) {
+ ret = -EIO;
}
}
qio_channel_set_cork(client->ioc, false);
@@ -985,7 +985,7 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
client->send_coroutine = NULL;
qemu_co_mutex_unlock(&client->send_lock);
- return rc;
+ return ret;
}
/* nbd_co_receive_request
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 12/12] nbd/server: refactor nbd_trip
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
` (10 preceding siblings ...)
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 11/12] nbd/server: rename rc to ret Vladimir Sementsov-Ogievskiy
@ 2017-06-02 15:01 ` Vladimir Sementsov-Ogievskiy
2017-06-13 18:04 ` Paolo Bonzini
2017-06-13 14:10 ` [Qemu-devel] ping Re: [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
12 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-02 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den, vsementsov
- do not use 'goto error_reply' outside a switch to jump into the
middle of the switch's default case label
- reduce code duplication
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 53 ++++++++++++++++++++---------------------------------
1 file changed, 20 insertions(+), 33 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 209a3d4e1e..198eabe338 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1078,6 +1078,7 @@ static coroutine_fn void nbd_trip(void *opaque)
NBDReply reply;
int ret;
int flags;
+ int reply_data_len = 0;
TRACE("Reading request.");
if (client->closing) {
@@ -1090,7 +1091,7 @@ static coroutine_fn void nbd_trip(void *opaque)
client->recv_coroutine = NULL;
nbd_client_receive_next_request(client);
if (ret == -EIO) {
- goto out;
+ goto disconnect;
}
reply.handle = request.handle;
@@ -1098,7 +1099,7 @@ static coroutine_fn void nbd_trip(void *opaque)
if (ret < 0) {
reply.error = -ret;
- goto error_reply;
+ goto reply;
}
if (client->closing) {
@@ -1119,7 +1120,7 @@ static coroutine_fn void nbd_trip(void *opaque)
if (ret < 0) {
LOG("flush failed");
reply.error = -ret;
- goto error_reply;
+ break;
}
}
@@ -1128,12 +1129,12 @@ static coroutine_fn void nbd_trip(void *opaque)
if (ret < 0) {
LOG("reading from file failed");
reply.error = -ret;
- goto error_reply;
+ break;
}
+ reply_data_len = request.len;
TRACE("Read %" PRIu32" byte(s)", request.len);
- if (nbd_co_send_reply(req, &reply, request.len) < 0)
- goto out;
+
break;
case NBD_CMD_WRITE:
TRACE("Request type is WRITE");
@@ -1141,7 +1142,7 @@ static coroutine_fn void nbd_trip(void *opaque)
if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
TRACE("Server is read-only, return error");
reply.error = EROFS;
- goto error_reply;
+ break;
}
TRACE("Writing to device");
@@ -1155,21 +1156,16 @@ static coroutine_fn void nbd_trip(void *opaque)
if (ret < 0) {
LOG("writing to file failed");
reply.error = -ret;
- goto error_reply;
}
- if (nbd_co_send_reply(req, &reply, 0) < 0) {
- goto out;
- }
break;
-
case NBD_CMD_WRITE_ZEROES:
TRACE("Request type is WRITE_ZEROES");
if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
TRACE("Server is read-only, return error");
reply.error = EROFS;
- goto error_reply;
+ break;
}
TRACE("Writing to device");
@@ -1186,14 +1182,9 @@ static coroutine_fn void nbd_trip(void *opaque)
if (ret < 0) {
LOG("writing to file failed");
reply.error = -ret;
- goto error_reply;
}
- if (nbd_co_send_reply(req, &reply, 0) < 0) {
- goto out;
- }
break;
-
case NBD_CMD_DISC:
/* unreachable, thanks to special case in nbd_co_receive_request() */
abort();
@@ -1206,9 +1197,7 @@ static coroutine_fn void nbd_trip(void *opaque)
LOG("flush failed");
reply.error = -ret;
}
- if (nbd_co_send_reply(req, &reply, 0) < 0) {
- goto out;
- }
+
break;
case NBD_CMD_TRIM:
TRACE("Request type is TRIM");
@@ -1218,21 +1207,19 @@ static coroutine_fn void nbd_trip(void *opaque)
LOG("discard failed");
reply.error = -ret;
}
- if (nbd_co_send_reply(req, &reply, 0) < 0) {
- goto out;
- }
+
break;
default:
LOG("invalid request type (%" PRIu32 ") received", request.type);
reply.error = EINVAL;
- error_reply:
- /* We must disconnect after NBD_CMD_WRITE if we did not
- * read the payload.
- */
- if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete) {
- goto out;
- }
- break;
+ }
+
+reply:
+ /* We must disconnect after NBD_CMD_WRITE if we did not
+ * read the payload.
+ */
+ if (nbd_co_send_reply(req, &reply, reply_data_len) < 0 || !req->complete) {
+ goto disconnect;
}
TRACE("Request/Reply complete");
@@ -1242,7 +1229,7 @@ done:
nbd_client_put(client);
return;
-out:
+disconnect:
nbd_request_put(req);
client_close(client);
nbd_client_put(client);
--
2.11.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v2 12/12] nbd/server: refactor nbd_trip
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 12/12] nbd/server: refactor nbd_trip Vladimir Sementsov-Ogievskiy
@ 2017-06-13 18:04 ` Paolo Bonzini
0 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2017-06-13 18:04 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel; +Cc: eblake, den
On 02/06/2017 17:01, Vladimir Sementsov-Ogievskiy wrote:
> - do not use 'goto error_reply' outside a switch to jump into the
> middle of the switch's default case label
> - reduce code duplication
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
As a followup, maybe the switch statement can be extracted to its own
function. This way the "goto reply" can be avoided. I'll take a look.
Apart from this, the series looks good. I'll queue it tomorrow.
Paolo
> ---
> nbd/server.c | 53 ++++++++++++++++++++---------------------------------
> 1 file changed, 20 insertions(+), 33 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index 209a3d4e1e..198eabe338 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1078,6 +1078,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> NBDReply reply;
> int ret;
> int flags;
> + int reply_data_len = 0;
>
> TRACE("Reading request.");
> if (client->closing) {
> @@ -1090,7 +1091,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> client->recv_coroutine = NULL;
> nbd_client_receive_next_request(client);
> if (ret == -EIO) {
> - goto out;
> + goto disconnect;
> }
>
> reply.handle = request.handle;
> @@ -1098,7 +1099,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>
> if (ret < 0) {
> reply.error = -ret;
> - goto error_reply;
> + goto reply;
> }
>
> if (client->closing) {
> @@ -1119,7 +1120,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> if (ret < 0) {
> LOG("flush failed");
> reply.error = -ret;
> - goto error_reply;
> + break;
> }
> }
>
> @@ -1128,12 +1129,12 @@ static coroutine_fn void nbd_trip(void *opaque)
> if (ret < 0) {
> LOG("reading from file failed");
> reply.error = -ret;
> - goto error_reply;
> + break;
> }
>
> + reply_data_len = request.len;
> TRACE("Read %" PRIu32" byte(s)", request.len);
> - if (nbd_co_send_reply(req, &reply, request.len) < 0)
> - goto out;
> +
> break;
> case NBD_CMD_WRITE:
> TRACE("Request type is WRITE");
> @@ -1141,7 +1142,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
> TRACE("Server is read-only, return error");
> reply.error = EROFS;
> - goto error_reply;
> + break;
> }
>
> TRACE("Writing to device");
> @@ -1155,21 +1156,16 @@ static coroutine_fn void nbd_trip(void *opaque)
> if (ret < 0) {
> LOG("writing to file failed");
> reply.error = -ret;
> - goto error_reply;
> }
>
> - if (nbd_co_send_reply(req, &reply, 0) < 0) {
> - goto out;
> - }
> break;
> -
> case NBD_CMD_WRITE_ZEROES:
> TRACE("Request type is WRITE_ZEROES");
>
> if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
> TRACE("Server is read-only, return error");
> reply.error = EROFS;
> - goto error_reply;
> + break;
> }
>
> TRACE("Writing to device");
> @@ -1186,14 +1182,9 @@ static coroutine_fn void nbd_trip(void *opaque)
> if (ret < 0) {
> LOG("writing to file failed");
> reply.error = -ret;
> - goto error_reply;
> }
>
> - if (nbd_co_send_reply(req, &reply, 0) < 0) {
> - goto out;
> - }
> break;
> -
> case NBD_CMD_DISC:
> /* unreachable, thanks to special case in nbd_co_receive_request() */
> abort();
> @@ -1206,9 +1197,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> LOG("flush failed");
> reply.error = -ret;
> }
> - if (nbd_co_send_reply(req, &reply, 0) < 0) {
> - goto out;
> - }
> +
> break;
> case NBD_CMD_TRIM:
> TRACE("Request type is TRIM");
> @@ -1218,21 +1207,19 @@ static coroutine_fn void nbd_trip(void *opaque)
> LOG("discard failed");
> reply.error = -ret;
> }
> - if (nbd_co_send_reply(req, &reply, 0) < 0) {
> - goto out;
> - }
> +
> break;
> default:
> LOG("invalid request type (%" PRIu32 ") received", request.type);
> reply.error = EINVAL;
> - error_reply:
> - /* We must disconnect after NBD_CMD_WRITE if we did not
> - * read the payload.
> - */
> - if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete) {
> - goto out;
> - }
> - break;
> + }
> +
> +reply:
> + /* We must disconnect after NBD_CMD_WRITE if we did not
> + * read the payload.
> + */
> + if (nbd_co_send_reply(req, &reply, reply_data_len) < 0 || !req->complete) {
> + goto disconnect;
> }
>
> TRACE("Request/Reply complete");
> @@ -1242,7 +1229,7 @@ done:
> nbd_client_put(client);
> return;
>
> -out:
> +disconnect:
> nbd_request_put(req);
> client_close(client);
> nbd_client_put(client);
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] ping Re: [PATCH v2 00/12] nbd refactoring part 1
2017-06-02 15:01 [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1 Vladimir Sementsov-Ogievskiy
` (11 preceding siblings ...)
2017-06-02 15:01 ` [Qemu-devel] [PATCH v2 12/12] nbd/server: refactor nbd_trip Vladimir Sementsov-Ogievskiy
@ 2017-06-13 14:10 ` Vladimir Sementsov-Ogievskiy
12 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-06-13 14:10 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, eblake, den
ping... What about this?
02.06.2017 18:01, Vladimir Sementsov-Ogievskiy wrote:
> This is based on my "nbd: error path refactoring" series.
> This is an update of first part of my "nbd errors and traces refactoring"
>
> v2:
> 01: update commit msg
> grammar in comment
> add Eric's r-b
> 02: add Eric's r-b
> 03: update commit msg
> add Eric's r-b
>
>
> v1:
> changes from patches 01-10 of "nbd errors and traces refactoring" (mostly
> proposed by Eric):
> 01 new patch
> 02-03 - split of old 01
> 08 and 12 - minor changes in commit messages
> 04-12 - add r-b by Eric
>
> Also, of course, renaming in 01 is "visible" in other patches.
>
> Vladimir Sementsov-Ogievskiy (12):
> nbd: rename read_sync and friends
> nbd: make nbd_drop public
> nbd/server: get rid of nbd_negotiate_read and friends
> nbd/server: get rid of ssize_t
> nbd/server: refactor nbd_co_send_reply
> nbd/server: get rid of EAGAIN dead code
> nbd/server: refactor nbd_co_receive_request
> nbd/server: remove NBDClientNewData
> nbd/server: nbd_negotiate: fix error path
> nbd/server: get rid of fail: return rc
> nbd/server: rename rc to ret
> nbd/server: refactor nbd_trip
>
> block/nbd-client.c | 8 +-
> include/block/nbd.h | 8 +-
> nbd/client.c | 64 ++++-------
> nbd/common.c | 34 +++++-
> nbd/nbd-internal.h | 28 ++---
> nbd/server.c | 319 +++++++++++++++++-----------------------------------
> 6 files changed, 172 insertions(+), 289 deletions(-)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 16+ messages in thread