Re: [Qemu-devel] [PATCH v2 10/10] nbd: Minimal structured read for client
10.10.2017 11:02, Paolo Bonzini wrote:
On 09/10/2017 19:27, Vladimir Sementsov-Ogievskiy wrote:
+int ret = nbd_co_do_receive_one_chunk(s, handle, only_structured,
+ &request_ret, qiov, payload);
+
+if (ret < 0) {
+s->quit = true;
+} else {
+/* For assert at loop start in nbd_read_reply_entry */
+if (reply) {
+*reply = s->reply;
+}
reply is always non-NULL, right?
+s->reply.handle = 0;
+ret = request_ret;
+}
...
+static int nbd_co_receive_return_code(NBDClientSession *s, uint64_t handle)
+{
+int ret = 0;
+int i = HANDLE_TO_INDEX(s, handle);
+NBDReply reply;
+bool only_structured = false;
+
+do {
+int rc = nbd_co_receive_one_chunk(s, handle, only_structured,
+ NULL, &reply, NULL);
Is rc > 0 propagated correctly?
+if (rc < 0 || nbd_reply_is_simple(&reply)) {
+if (ret == 0) {
+ret = rc;
+}
+only_structured = true;
+continue;
+}
+} while (!s->quit && nbd_reply_is_structured(&s->reply) &&
+ !(s->reply.structured.flags & NBD_SREP_FLAG_DONE));
+
+s->requests[i].coroutine = NULL;
+
+qemu_co_mutex_lock(&s->send_mutex);
+s->in_flight--;
+qemu_co_queue_next(&s->free_sema);
+qemu_co_mutex_unlock(&s->send_mutex);
The code after the loop is duplicated.
An idea could be to wrap nbd_co_receive_one_chunk with an iterator like
typedef struct NBDReplyChunkIter {
int ret;
bool done, only_structured;
} NBDReplyChunkIter;
#define NBD_FOREACH_REPLY_CHUNK(s, handle, iter, qiov, reply, payload, \
structured)\
for (iter = (NBDReplyChunkIter) { .only_structured = structured }; \
nbd_reply_chunk_iter_receive(s, &iter, qiov, &reply, handle, \
payload);;)
bool coroutine_fn nbd_reply_chunk_iter_receive(...)
{
if (s->quit) {
if (iter->ret == 0) {
iter->ret = -EIO;
}
goto break_loop;
}
/* Handle the end of the previous iteration. */
if (iter->done) {
goto break_loop;
}
rc = nbd_co_receive_one_chunk(s, handle, iter->only_structured,
qiov, reply, payload);
if (rc < 0) {
assert(s->quit);
if (iter->ret == 0) {
iter->ret = rc;
}
goto break_loop;
}
/* No structured reply? Do not execute the body of
* NBD_FOREACH_REPLY_CHUNK.
*/
if (nbd_reply_is_simple(&s->reply) || s->quit) {
goto break_loop;
}
iter->only_structured = true;
if (s->reply.structured.flags & NBD_SREP_FLAG_DONE) {
iter->ret = rc;
iter->done = true;
/* Execute the loop body, but this iteration is the last. */
return true;
}
/* An error reply must have NBD_SREP_FLAG_DONE set, otherwise it is
* a protocol error (FIXME: is this true? but if not, how do you
* cope with multiple error replies?)
unfortunately it's not true, spec doesn't forbid several error replies
on one request.
Furthermore, we have NBD_REPLY_TYPE_ERROR_OFFSET, which have ability of
several error chunks it its nature.
*/
if (rc > 0) {
s->quit = true;
iter->ret = -EIO;
goto break_loop;
}
return true;
break_loop:
iter->done = true;
s->requests[HANDLE_TO_INDEX(s, handle)].coroutine = NULL;
qemu_co_mutex_lock(&s->send_mutex);
s->in_flight--;
qemu_co_queue_next(&s->free_sema);
qemu_co_mutex_unlock(&s->send_mutex);
return false;
}
so this loop could be written like
FOR_EACH_REPLY_CHUNK(s, handle, iter, NULL, &reply, NULL) {
if (reply.structured.type != NBD_SREP_TYPE_NONE) {
/* not allowed reply type */
s->quit = true;
break;
}
}
return iter.ret;
and likewise for nbd_co_receive_cmdread_reply. The iterator is a bit
more complex, but it abstracts all the protocol handling and avoids lots
of duplicated code.
Paolo
+return ret;
+}
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH v2 10/10] nbd: Minimal structured read for client
10.10.2017 18:00, Paolo Bonzini wrote: On 10/10/2017 16:55, Vladimir Sementsov-Ogievskiy wrote: Hmm, would it be simpler just pass a function pointer, which should be called on each loop iteration? So, we will return to one common func nbd_co_receive_reply, but with two additional parameters: func and opaque? Function pointers typically result in having to pass the state around in a structure, for all the callers. An iterator also has to package the state in a structure, but it is only done once. So function pointers would be simpler in the beginning, but would not scale as well. Paolo Understand. OK, I'll try your suggestion. -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH v2 10/10] nbd: Minimal structured read for client
On 10/10/2017 16:55, Vladimir Sementsov-Ogievskiy wrote: > Hmm, would it be simpler just pass a function pointer, which should be > called on each loop iteration? > So, we will return to one common func nbd_co_receive_reply, but with two > additional parameters: func and opaque? Function pointers typically result in having to pass the state around in a structure, for all the callers. An iterator also has to package the state in a structure, but it is only done once. So function pointers would be simpler in the beginning, but would not scale as well. Paolo
Re: [Qemu-devel] [PATCH v2 10/10] nbd: Minimal structured read for client
10.10.2017 11:02, Paolo Bonzini wrote:
On 09/10/2017 19:27, Vladimir Sementsov-Ogievskiy wrote:
+int ret = nbd_co_do_receive_one_chunk(s, handle, only_structured,
+ &request_ret, qiov, payload);
+
+if (ret < 0) {
+s->quit = true;
+} else {
+/* For assert at loop start in nbd_read_reply_entry */
+if (reply) {
+*reply = s->reply;
+}
reply is always non-NULL, right?
+s->reply.handle = 0;
+ret = request_ret;
+}
...
+static int nbd_co_receive_return_code(NBDClientSession *s, uint64_t handle)
+{
+int ret = 0;
+int i = HANDLE_TO_INDEX(s, handle);
+NBDReply reply;
+bool only_structured = false;
+
+do {
+int rc = nbd_co_receive_one_chunk(s, handle, only_structured,
+ NULL, &reply, NULL);
Is rc > 0 propagated correctly?
+if (rc < 0 || nbd_reply_is_simple(&reply)) {
+if (ret == 0) {
+ret = rc;
+}
+only_structured = true;
+continue;
+}
+} while (!s->quit && nbd_reply_is_structured(&s->reply) &&
+ !(s->reply.structured.flags & NBD_SREP_FLAG_DONE));
+
+s->requests[i].coroutine = NULL;
+
+qemu_co_mutex_lock(&s->send_mutex);
+s->in_flight--;
+qemu_co_queue_next(&s->free_sema);
+qemu_co_mutex_unlock(&s->send_mutex);
The code after the loop is duplicated.
An idea could be to wrap nbd_co_receive_one_chunk with an iterator like
typedef struct NBDReplyChunkIter {
int ret;
bool done, only_structured;
} NBDReplyChunkIter;
#define NBD_FOREACH_REPLY_CHUNK(s, handle, iter, qiov, reply, payload, \
structured)\
for (iter = (NBDReplyChunkIter) { .only_structured = structured }; \
nbd_reply_chunk_iter_receive(s, &iter, qiov, &reply, handle, \
payload);;)
bool coroutine_fn nbd_reply_chunk_iter_receive(...)
{
if (s->quit) {
if (iter->ret == 0) {
iter->ret = -EIO;
}
goto break_loop;
}
/* Handle the end of the previous iteration. */
if (iter->done) {
goto break_loop;
}
rc = nbd_co_receive_one_chunk(s, handle, iter->only_structured,
qiov, reply, payload);
if (rc < 0) {
assert(s->quit);
if (iter->ret == 0) {
iter->ret = rc;
}
goto break_loop;
}
/* No structured reply? Do not execute the body of
* NBD_FOREACH_REPLY_CHUNK.
*/
if (nbd_reply_is_simple(&s->reply) || s->quit) {
goto break_loop;
}
iter->only_structured = true;
if (s->reply.structured.flags & NBD_SREP_FLAG_DONE) {
iter->ret = rc;
iter->done = true;
/* Execute the loop body, but this iteration is the last. */
return true;
}
/* An error reply must have NBD_SREP_FLAG_DONE set, otherwise it is
* a protocol error (FIXME: is this true? but if not, how do you
* cope with multiple error replies?)
*/
if (rc > 0) {
s->quit = true;
iter->ret = -EIO;
goto break_loop;
}
return true;
break_loop:
iter->done = true;
s->requests[HANDLE_TO_INDEX(s, handle)].coroutine = NULL;
qemu_co_mutex_lock(&s->send_mutex);
s->in_flight--;
qemu_co_queue_next(&s->free_sema);
qemu_co_mutex_unlock(&s->send_mutex);
return false;
}
so this loop could be written like
FOR_EACH_REPLY_CHUNK(s, handle, iter, NULL, &reply, NULL) {
if (reply.structured.type != NBD_SREP_TYPE_NONE) {
/* not allowed reply type */
s->quit = true;
break;
}
}
return iter.ret;
and likewise for nbd_co_receive_cmdread_reply. The iterator is a bit
more complex, but it abstracts all the protocol handling and avoids lots
of duplicated code.
Paolo
+return ret;
+}
Hmm, would it be simpler just pass a function pointer, which should be
called on each loop iteration?
So, we will return to one common func nbd_co_receive_reply, but with two
additional parameters: func and opaque?
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH v2 10/10] nbd: Minimal structured read for client
09.10.2017 20:27, Vladimir Sementsov-Ogievskiy wrote:
Minimal implementation: for structured error only error_report error
message.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/nbd.h | 6 +
block/nbd-client.c | 358
nbd/client.c| 7 +
3 files changed, 343 insertions(+), 28 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 1ef8c8897f..e3350b67a4 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -203,6 +203,11 @@ enum {
#define NBD_SREP_TYPE_ERROR NBD_SREP_ERR(1)
#define NBD_SREP_TYPE_ERROR_OFFSET NBD_SREP_ERR(2)
+static inline bool nbd_srep_type_is_error(int type)
+{
+return type & (1 << 15);
+}
+
/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
* but only a limited set of errno values is specified in the protocol.
* Everything else is squashed to EINVAL.
@@ -241,6 +246,7 @@ static inline int nbd_errno_to_system_errno(int err)
struct NBDExportInfo {
/* Set by client before nbd_receive_negotiate() */
bool request_sizes;
+bool structured_reply;
/* Set by server results during nbd_receive_negotiate() */
uint64_t size;
uint16_t flags;
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 58493b7ac4..eda78130a2 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -29,6 +29,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qemu/error-report.h"
#include "nbd-client.h"
#define HANDLE_TO_INDEX(bs, handle) ((handle) ^ (uint64_t)(intptr_t)(bs))
@@ -93,7 +94,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
if (i >= MAX_NBD_REQUESTS ||
!s->requests[i].coroutine ||
!s->requests[i].receiving ||
-nbd_reply_is_structured(&s->reply))
+(nbd_reply_is_structured(&s->reply) && !s->info.structured_reply))
{
break;
}
@@ -181,41 +182,336 @@ err:
return rc;
}
-static int nbd_co_receive_reply(NBDClientSession *s,
-uint64_t handle,
-QEMUIOVector *qiov)
+static inline void payload_advance16(uint8_t **payload, uint16_t **ptr)
+{
+*ptr = (uint16_t *)*payload;
+be16_to_cpus(*ptr);
+*payload += sizeof(**ptr);
+}
+
+static inline void payload_advance32(uint8_t **payload, uint32_t **ptr)
+{
+*ptr = (uint32_t *)*payload;
+be32_to_cpus(*ptr);
+*payload += sizeof(**ptr);
+}
+
+static inline void payload_advance64(uint8_t **payload, uint64_t **ptr)
+{
+*ptr = (uint64_t *)*payload;
+be64_to_cpus(*ptr);
+*payload += sizeof(**ptr);
+}
+
+static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
+ uint8_t *payload, QEMUIOVector *qiov)
+{
+uint64_t *offset;
+uint32_t *hole_size;
+
+if (chunk->length != sizeof(*offset) + sizeof(*hole_size)) {
+return -EINVAL;
+}
+
+payload_advance64(&payload, &offset);
+payload_advance32(&payload, &hole_size);
+
+if (*offset + *hole_size > qiov->size) {
+return -EINVAL;
+}
+
+qemu_iovec_memset(qiov, *offset, 0, *hole_size);
+
+return 0;
+}
+
+static int nbd_parse_error_payload(NBDStructuredReplyChunk *chunk,
+ uint8_t *payload, int *request_ret)
+{
+uint32_t *error;
+uint16_t *message_size;
+
+assert(chunk->type & (1 << 15));
+
+if (chunk->length < sizeof(error) + sizeof(message_size)) {
+return -EINVAL;
+}
+
+payload_advance32(&payload, &error);
+payload_advance16(&payload, &message_size);
+
+error_report("%.*s", *message_size, payload);
+
+/* TODO add special case for ERROR_OFFSET */
+
+*request_ret = nbd_errno_to_system_errno(*error);
+
+return 0;
+}
+
+static int nbd_co_receive_offset_data_payload(NBDClientSession *s,
+ QEMUIOVector *qiov)
+{
+QEMUIOVector sub_qiov;
+uint64_t offset;
+size_t data_size;
+int ret;
+NBDStructuredReplyChunk *chunk = &s->reply.structured;
+
+assert(nbd_reply_is_structured(&s->reply));
+
+if (chunk->length < sizeof(offset)) {
+return -EINVAL;
+}
+
+if (nbd_read(s->ioc, &offset, sizeof(offset), NULL) < 0) {
+return -EIO;
+}
+be64_to_cpus(&offset);
+
+data_size = chunk->length - sizeof(offset);
+if (offset + data_size > qiov->size) {
+return -EINVAL;
+}
+
+qemu_iovec_init(&sub_qiov, qiov->niov);
+qemu_iovec_concat(&sub_qiov, qiov, offset, data_size);
+ret = qio_channel_readv_all(s->ioc, sub_qiov.iov, sub_qiov.niov, NULL);
+qemu_iovec_destroy(&sub_qiov);
+
+return ret < 0 ? -EIO : 0;
+}
+
+#define NBD_MAX_MALLOC_PAYLOAD 1000
+static int nbd_co_receive_structured_payload(NBDClientSession *s,
+ void **payload)
+{
+int ret;
+uint32_t l
Re: [Qemu-devel] [PATCH v2 10/10] nbd: Minimal structured read for client
On 09/10/2017 19:27, Vladimir Sementsov-Ogievskiy wrote:
>
> +int ret = nbd_co_do_receive_one_chunk(s, handle, only_structured,
> + &request_ret, qiov, payload);
> +
> +if (ret < 0) {
> +s->quit = true;
> +} else {
> +/* For assert at loop start in nbd_read_reply_entry */
> +if (reply) {
> +*reply = s->reply;
> +}
reply is always non-NULL, right?
> +s->reply.handle = 0;
> +ret = request_ret;
> +}
>
...
> +static int nbd_co_receive_return_code(NBDClientSession *s, uint64_t handle)
> +{
> +int ret = 0;
> +int i = HANDLE_TO_INDEX(s, handle);
> +NBDReply reply;
> +bool only_structured = false;
> +
> +do {
> +int rc = nbd_co_receive_one_chunk(s, handle, only_structured,
> + NULL, &reply, NULL);
Is rc > 0 propagated correctly?
> +if (rc < 0 || nbd_reply_is_simple(&reply)) {
> +if (ret == 0) {
> +ret = rc;
> +}
> +only_structured = true;
> +continue;
> +}
> +} while (!s->quit && nbd_reply_is_structured(&s->reply) &&
> + !(s->reply.structured.flags & NBD_SREP_FLAG_DONE));
> +
> +s->requests[i].coroutine = NULL;
> +
> +qemu_co_mutex_lock(&s->send_mutex);
> +s->in_flight--;
> +qemu_co_queue_next(&s->free_sema);
> +qemu_co_mutex_unlock(&s->send_mutex);
The code after the loop is duplicated.
An idea could be to wrap nbd_co_receive_one_chunk with an iterator like
typedef struct NBDReplyChunkIter {
int ret;
bool done, only_structured;
} NBDReplyChunkIter;
#define NBD_FOREACH_REPLY_CHUNK(s, handle, iter, qiov, reply, payload, \
structured)\
for (iter = (NBDReplyChunkIter) { .only_structured = structured }; \
nbd_reply_chunk_iter_receive(s, &iter, qiov, &reply, handle, \
payload);;)
bool coroutine_fn nbd_reply_chunk_iter_receive(...)
{
if (s->quit) {
if (iter->ret == 0) {
iter->ret = -EIO;
}
goto break_loop;
}
/* Handle the end of the previous iteration. */
if (iter->done) {
goto break_loop;
}
rc = nbd_co_receive_one_chunk(s, handle, iter->only_structured,
qiov, reply, payload);
if (rc < 0) {
assert(s->quit);
if (iter->ret == 0) {
iter->ret = rc;
}
goto break_loop;
}
/* No structured reply? Do not execute the body of
* NBD_FOREACH_REPLY_CHUNK.
*/
if (nbd_reply_is_simple(&s->reply) || s->quit) {
goto break_loop;
}
iter->only_structured = true;
if (s->reply.structured.flags & NBD_SREP_FLAG_DONE) {
iter->ret = rc;
iter->done = true;
/* Execute the loop body, but this iteration is the last. */
return true;
}
/* An error reply must have NBD_SREP_FLAG_DONE set, otherwise it is
* a protocol error (FIXME: is this true? but if not, how do you
* cope with multiple error replies?)
*/
if (rc > 0) {
s->quit = true;
iter->ret = -EIO;
goto break_loop;
}
return true;
break_loop:
iter->done = true;
s->requests[HANDLE_TO_INDEX(s, handle)].coroutine = NULL;
qemu_co_mutex_lock(&s->send_mutex);
s->in_flight--;
qemu_co_queue_next(&s->free_sema);
qemu_co_mutex_unlock(&s->send_mutex);
return false;
}
so this loop could be written like
FOR_EACH_REPLY_CHUNK(s, handle, iter, NULL, &reply, NULL) {
if (reply.structured.type != NBD_SREP_TYPE_NONE) {
/* not allowed reply type */
s->quit = true;
break;
}
}
return iter.ret;
and likewise for nbd_co_receive_cmdread_reply. The iterator is a bit
more complex, but it abstracts all the protocol handling and avoids lots
of duplicated code.
Paolo
> +return ret;
> +}
