[Qemu-block] [PATCH v2 4/7] block/nbd-client: drop reply field from NBDClientSession

2017-10-09 Thread Vladimir Sementsov-Ogievskiy 
Drop 'reply' from NBDClientSession. It's used to only deliver request
return code to receiving coroutine. Instead introduce per-request ret
variable to simplify the scheme and make further refactoring possible.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd-client.h |  2 +-
 block/nbd-client.c | 22 +-
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index 3f97d31013..4bc8fe3993 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -22,6 +22,7 @@ typedef struct {
 bool receiving; /* waiting for read_reply_co? */
 NBDRequest *request;
 QEMUIOVector *qiov;
+int ret;
 } NBDClientRequest;
 
 typedef struct NBDClientSession {
@@ -35,7 +36,6 @@ typedef struct NBDClientSession {
 int in_flight;
 
 NBDClientRequest requests[MAX_NBD_REQUESTS];
-NBDReply reply;
 bool quit;
 } NBDClientSession;
 
diff --git a/block/nbd-client.c b/block/nbd-client.c
index f7b642f079..f331f08a9a 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -74,10 +74,10 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 uint64_t i;
 int ret = 0;
 Error *local_err = NULL;
+NBDReply reply;
 
 while (!s->quit) {
-assert(s->reply.handle == 0);
-ret = nbd_receive_reply(s->ioc, >reply, _err);
+ret = nbd_receive_reply(s->ioc, , _err);
 if (ret < 0) {
 error_report_err(local_err);
 }
@@ -89,18 +89,18 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  * handler acts as a synchronization point and ensures that only
  * one coroutine is called until the reply finishes.
  */
-i = HANDLE_TO_INDEX(s, s->reply.handle);
+i = HANDLE_TO_INDEX(s, reply.handle);
 if (i >= MAX_NBD_REQUESTS ||
 !s->requests[i].coroutine ||
 !s->requests[i].receiving ||
-s->reply.handle != s->requests[i].request->handle)
+reply.handle != s->requests[i].request->handle)
 {
 break;
 }
 
-if (s->reply.error == 0 &&
-s->requests[i].request->type == NBD_CMD_READ)
-{
+s->requests[i].ret = -reply.error;
+
+if (reply.error == 0 && s->requests[i].request->type == NBD_CMD_READ) {
 QEMUIOVector *qiov = s->requests[i].qiov;
 assert(qiov != NULL);
 assert(s->requests[i].request->len ==
@@ -108,7 +108,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
   NULL) < 0)
 {
-s->reply.error = EIO;
+s->requests[i].ret = -EIO;
 break;
 }
 }
@@ -211,11 +211,7 @@ static int nbd_co_receive_reply(NBDClientSession *s, 
NBDRequest *request)
 if (!s->ioc || s->quit) {
 ret = -EIO;
 } else {
-assert(s->reply.handle == request->handle);
-ret = -s->reply.error;
-
-/* Tell the read handler to read another header.  */
-s->reply.handle = 0;
+ret = s->requests[i].ret;
 }
 
 s->requests[i].coroutine = NULL;
-- 
2.11.1

Re: [Qemu-block] [PATCH v2 4/7] block/nbd-client: drop reply field from NBDClientSession

2017-09-18 Thread Eric Blake 
On 09/18/2017 08:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> Drop 'reply' from NBDClientSession. It's used to only deliver request
> return code to receiving coroutine. Instead introduce per-request ret
> variable to simplify the scheme and make further refactoring possible.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd-client.h |  2 +-
>  block/nbd-client.c | 22 +-
>  2 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index 3f97d31013..4bc8fe3993 100644
> --- a/block/nbd-client.h
> +++ b/block/nbd-client.h
> @@ -22,6 +22,7 @@ typedef struct {
>  bool receiving; /* waiting for read_reply_co? */
>  NBDRequest *request;
>  QEMUIOVector *qiov;
> +int ret;
>  } NBDClientRequest;

I like this idea.  However, I note that:

> @@ -211,11 +211,7 @@ static int nbd_co_receive_reply(NBDClientSession *s, 
> NBDRequest *request)
>  if (!s->ioc || s->quit) {
>  ret = -EIO;
>  } else {
> -assert(s->reply.handle == request->handle);
> -ret = -s->reply.error;
> -
> -/* Tell the read handler to read another header.  */
> -s->reply.handle = 0;
> +ret = s->requests[i].ret;

you are removing the assert you added in 2/7, where I questioned whether
we needed NBDClientRequest.request in the first place.  So there may be
some rebase churn, depending on how that conversation pans out.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v2 4/7] block/nbd-client: drop reply field from NBDClientSession

2017-09-18 Thread Vladimir Sementsov-Ogievskiy 
Drop 'reply' from NBDClientSession. It's used to only deliver request
return code to receiving coroutine. Instead introduce per-request ret
variable to simplify the scheme and make further refactoring possible.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd-client.h |  2 +-
 block/nbd-client.c | 22 +-
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index 3f97d31013..4bc8fe3993 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -22,6 +22,7 @@ typedef struct {
 bool receiving; /* waiting for read_reply_co? */
 NBDRequest *request;
 QEMUIOVector *qiov;
+int ret;
 } NBDClientRequest;
 
 typedef struct NBDClientSession {
@@ -35,7 +36,6 @@ typedef struct NBDClientSession {
 int in_flight;
 
 NBDClientRequest requests[MAX_NBD_REQUESTS];
-NBDReply reply;
 bool quit;
 } NBDClientSession;
 
diff --git a/block/nbd-client.c b/block/nbd-client.c
index f7b642f079..f331f08a9a 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -74,10 +74,10 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 uint64_t i;
 int ret = 0;
 Error *local_err = NULL;
+NBDReply reply;
 
 while (!s->quit) {
-assert(s->reply.handle == 0);
-ret = nbd_receive_reply(s->ioc, >reply, _err);
+ret = nbd_receive_reply(s->ioc, , _err);
 if (ret < 0) {
 error_report_err(local_err);
 }
@@ -89,18 +89,18 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  * handler acts as a synchronization point and ensures that only
  * one coroutine is called until the reply finishes.
  */
-i = HANDLE_TO_INDEX(s, s->reply.handle);
+i = HANDLE_TO_INDEX(s, reply.handle);
 if (i >= MAX_NBD_REQUESTS ||
 !s->requests[i].coroutine ||
 !s->requests[i].receiving ||
-s->reply.handle != s->requests[i].request->handle)
+reply.handle != s->requests[i].request->handle)
 {
 break;
 }
 
-if (s->reply.error == 0 &&
-s->requests[i].request->type == NBD_CMD_READ)
-{
+s->requests[i].ret = -reply.error;
+
+if (reply.error == 0 && s->requests[i].request->type == NBD_CMD_READ) {
 QEMUIOVector *qiov = s->requests[i].qiov;
 assert(qiov != NULL);
 assert(s->requests[i].request->len ==
@@ -108,7 +108,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
   NULL) < 0)
 {
-s->reply.error = EIO;
+s->requests[i].ret = -EIO;
 break;
 }
 }
@@ -211,11 +211,7 @@ static int nbd_co_receive_reply(NBDClientSession *s, 
NBDRequest *request)
 if (!s->ioc || s->quit) {
 ret = -EIO;
 } else {
-assert(s->reply.handle == request->handle);
-ret = -s->reply.error;
-
-/* Tell the read handler to read another header.  */
-s->reply.handle = 0;
+ret = s->requests[i].ret;
 }
 
 s->requests[i].coroutine = NULL;
-- 
2.11.1