[Qemu-devel] [PATCH v3 0/3] nbd client refactoring and fixing

[Qemu-devel] [PATCH v3 0/3] nbd client refactoring and fixing

[Vladimir Sementsov-Ogievskiy]

v3: dropped all controversial things. I'll try to implement minimal
structured reply and block status over this small residue.

Vladimir Sementsov-Ogievskiy (3):
  block/nbd-client: refactor nbd_co_receive_reply
  block/nbd-client: simplify check in nbd_co_receive_reply
  block/nbd-client: nbd_co_send_request: fix return code

 block/nbd-client.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH v3 1/3] block/nbd-client: refactor nbd_co_receive_reply

[Vladimir Sementsov-Ogievskiy]

"NBDReply *reply" parameter of nbd_co_receive_reply is used only
to pass return value for nbd_co_request (reply.error). Remove it
and use function return value instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index ee7f758e68..acd8e5d007 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -178,26 +178,26 @@ err:
     return rc;
 }
 
-static void nbd_co_receive_reply(NBDClientSession *s,
-                                 NBDRequest *request,
-                                 NBDReply *reply,
-                                 QEMUIOVector *qiov)
+static int nbd_co_receive_reply(NBDClientSession *s,
+                                NBDRequest *request,
+                                QEMUIOVector *qiov)
 {
+    int ret;
     int i = HANDLE_TO_INDEX(s, request->handle);
 
     /* Wait until we're woken up by nbd_read_reply_entry.  */
     s->requests[i].receiving = true;
     qemu_coroutine_yield();
     s->requests[i].receiving = false;
-    *reply = s->reply;
-    if (reply->handle != request->handle || !s->ioc || s->quit) {
-        reply->error = EIO;
+    if (s->reply.handle != request->handle || !s->ioc || s->quit) {
+        ret = -EIO;
     } else {
-        if (qiov && reply->error == 0) {
+        ret = -s->reply.error;
+        if (qiov && s->reply.error == 0) {
             assert(request->len == iov_size(qiov->iov, qiov->niov));
             if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
                                       NULL) < 0) {
-                reply->error = EIO;
+                ret = -EIO;
                 s->quit = true;
             }
         }
@@ -217,6 +217,8 @@ static void nbd_co_receive_reply(NBDClientSession *s,
     s->in_flight--;
     qemu_co_queue_next(&s->free_sema);
     qemu_co_mutex_unlock(&s->send_mutex);
+
+    return ret;
 }
 
 static int nbd_co_request(BlockDriverState *bs,
@@ -224,7 +226,6 @@ static int nbd_co_request(BlockDriverState *bs,
                           QEMUIOVector *qiov)
 {
     NBDClientSession *client = nbd_get_client_session(bs);
-    NBDReply reply;
     int ret;
 
     assert(!qiov || request->type == NBD_CMD_WRITE ||
@@ -232,12 +233,11 @@ static int nbd_co_request(BlockDriverState *bs,
     ret = nbd_co_send_request(bs, request,
                               request->type == NBD_CMD_WRITE ? qiov : NULL);
     if (ret < 0) {
-        reply.error = -ret;
-    } else {
-        nbd_co_receive_reply(client, request, &reply,
-                             request->type == NBD_CMD_READ ? qiov : NULL);
+        return ret;
     }
-    return -reply.error;
+
+    return nbd_co_receive_reply(client, request,
+                                request->type == NBD_CMD_READ ? qiov : NULL);
 }
 
 int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
-- 
2.11.1

[Qemu-devel] [PATCH v3 2/3] block/nbd-client: simplify check in nbd_co_receive_reply

[Vladimir Sementsov-Ogievskiy]

If we are woken up from while() loop in nbd_read_reply_entry
handles must be equal. If we are woken up from
nbd_recv_coroutines_wake_all s->quit must be true, so we do
not need checking handles equality.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index acd8e5d007..486bfff9f7 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -189,9 +189,10 @@ static int nbd_co_receive_reply(NBDClientSession *s,
     s->requests[i].receiving = true;
     qemu_coroutine_yield();
     s->requests[i].receiving = false;
-    if (s->reply.handle != request->handle || !s->ioc || s->quit) {
+    if (!s->ioc || s->quit) {
         ret = -EIO;
     } else {
+        assert(s->reply.handle == request->handle);
         ret = -s->reply.error;
         if (qiov && s->reply.error == 0) {
             assert(request->len == iov_size(qiov->iov, qiov->niov));
-- 
2.11.1

Re: [Qemu-devel] [PATCH v3 2/3] block/nbd-client: simplify check in nbd_co_receive_reply

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On 09/20/2017 07:45 AM, Vladimir Sementsov-Ogievskiy wrote:
> If we are woken up from while() loop in nbd_read_reply_entry
> handles must be equal. If we are woken up from
> nbd_recv_coroutines_wake_all s->quit must be true, so we do
> not need checking handles equality.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd-client.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index acd8e5d007..486bfff9f7 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -189,9 +189,10 @@ static int nbd_co_receive_reply(NBDClientSession *s,
>      s->requests[i].receiving = true;
>      qemu_coroutine_yield();
>      s->requests[i].receiving = false;
> -    if (s->reply.handle != request->handle || !s->ioc || s->quit) {
> +    if (!s->ioc || s->quit) {
>          ret = -EIO;
>      } else {
> +        assert(s->reply.handle == request->handle);
>          ret = -s->reply.error;
>          if (qiov && s->reply.error == 0) {
>              assert(request->len == iov_size(qiov->iov, qiov->niov));
> 

-- 
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 --]

[Qemu-devel] [PATCH v3 3/3] block/nbd-client: nbd_co_send_request: fix return code

[Vladimir Sementsov-Ogievskiy]

It's incorrect to return success rc >= 0 if we skip qio_channel_writev_all()
call due to s->quit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 486bfff9f7..9d1e154feb 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -161,6 +161,8 @@ static int nbd_co_send_request(BlockDriverState *bs,
                                        NULL) < 0) {
                 rc = -EIO;
             }
+        } else if (rc >= 0) {
+            rc = -EIO;
         }
         qio_channel_set_cork(s->ioc, false);
     } else {
-- 
2.11.1

Re: [Qemu-devel] [PATCH v3 3/3] block/nbd-client: nbd_co_send_request: fix return code

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On 09/20/2017 07:45 AM, Vladimir Sementsov-Ogievskiy wrote:
> It's incorrect to return success rc >= 0 if we skip qio_channel_writev_all()
> call due to s->quit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd-client.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 486bfff9f7..9d1e154feb 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -161,6 +161,8 @@ static int nbd_co_send_request(BlockDriverState *bs,
>                                         NULL) < 0) {
>                  rc = -EIO;
>              }
> +        } else if (rc >= 0) {
> +            rc = -EIO;
>          }
>          qio_channel_set_cork(s->ioc, false);
>      } else {
> 

-- 
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 --]

Re: [Qemu-devel] [PATCH v3 0/3] nbd client refactoring and fixing

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On 09/20/2017 07:45 AM, Vladimir Sementsov-Ogievskiy wrote:
> v3: dropped all controversial things. I'll try to implement minimal
> structured reply and block status over this small residue.
> 
> Vladimir Sementsov-Ogievskiy (3):
>   block/nbd-client: refactor nbd_co_receive_reply
>   block/nbd-client: simplify check in nbd_co_receive_reply
>   block/nbd-client: nbd_co_send_request: fix return code

Thanks; queued on my NBD tree, will send pull request after a bit more
time to see if it collects further reviews:
git://repo.or.cz/qemu/ericb.git nbd

-- 
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 --]

Re: [Qemu-devel] [PATCH v3 0/3] nbd client refactoring and fixing

[Vladimir Sementsov-Ogievskiy]

20.09.2017 20:22, Eric Blake wrote:
> On 09/20/2017 07:45 AM, Vladimir Sementsov-Ogievskiy wrote:
>> v3: dropped all controversial things. I'll try to implement minimal
>> structured reply and block status over this small residue.
>>
>> Vladimir Sementsov-Ogievskiy (3):
>>    block/nbd-client: refactor nbd_co_receive_reply
>>    block/nbd-client: simplify check in nbd_co_receive_reply
>>    block/nbd-client: nbd_co_send_request: fix return code
> Thanks; queued on my NBD tree, will send pull request after a bit more
> time to see if it collects further reviews:
> git://repo.or.cz/qemu/ericb.git nbd
>

Thank you, Eric!


-- 
Best regards,
Vladimir