diff mbox

[07/17] block/nbd-client: refactor request send/receive

Message ID 20170804151440.320927-8-vsementsov@virtuozzo.com
State New
Headers show

Commit Message

Vladimir Sementsov-Ogievskiy Aug. 4, 2017, 3:14 p.m. UTC 
Move nbd_co_receive_reply and nbd_coroutine_end calls into
nbd_co_send_request and rename the latter to just nbd_co_request.

This removes code duplications in nbd_client_co_{pwrite,pread,...}
functions. Also this is needed for further refactoring.

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

Comments

Eric Blake Aug. 25, 2017, 6:49 p.m. UTC | #1 
On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move nbd_co_receive_reply and nbd_coroutine_end calls into
> nbd_co_send_request and rename the latter to just nbd_co_request.
> 
> This removes code duplications in nbd_client_co_{pwrite,pread,...}
> functions. Also this is needed for further refactoring.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd-client.c | 89 ++++++++++++++++++++----------------------------------
>  1 file changed, 33 insertions(+), 56 deletions(-)

The diffstat shows this is a nice improvement.
> +++ b/block/nbd-client.c
> @@ -112,12 +112,20 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
>      s->read_reply_co = NULL;
>  }
>  
> -static int nbd_co_send_request(BlockDriverState *bs,
> -                               NBDRequest *request,
> -                               QEMUIOVector *qiov)
> +static void nbd_co_receive_reply(NBDClientSession *s,
> +                                 NBDRequest *request,
> +                                 NBDReply *reply,
> +                                 QEMUIOVector *qiov);
> +static void nbd_coroutine_end(BlockDriverState *bs,
> +                              NBDRequest *request);

Is it possible to organize the functions in topological order so that we
don't need forward declarations of static functions?  (If there is
mutual recursion, you need the forward declaration; but other than that,
I like reading the building blocks first rather than skipping around)

Otherwise,
Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake Aug. 25, 2017, 7:08 p.m. UTC | #2 
On 08/25/2017 01:49 PM, Eric Blake wrote:
> On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Move nbd_co_receive_reply and nbd_coroutine_end calls into
>> nbd_co_send_request and rename the latter to just nbd_co_request.
>>
>> This removes code duplications in nbd_client_co_{pwrite,pread,...}
>> functions. Also this is needed for further refactoring.
>>

>> -static int nbd_co_send_request(BlockDriverState *bs,
>> -                               NBDRequest *request,
>> -                               QEMUIOVector *qiov)
>> +static void nbd_co_receive_reply(NBDClientSession *s,
>> +                                 NBDRequest *request,
>> +                                 NBDReply *reply,
>> +                                 QEMUIOVector *qiov);
>> +static void nbd_coroutine_end(BlockDriverState *bs,
>> +                              NBDRequest *request);
> 
> Is it possible to organize the functions in topological order so that we
> don't need forward declarations of static functions?  (If there is
> mutual recursion, you need the forward declaration; but other than that,
> I like reading the building blocks first rather than skipping around)

Answering myself: patch 9 inlines nbd_co_receive_reply into its lone
caller, eliminating the need for a forward reference, and it is less
code churn to have a temporary forward declaration than it is to move
the function body up and then back down.  (Maybe the commit message
could hint that nbd_co_receive_reply will later be inlined)

> 
> Otherwise,
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
diff mbox

Patch

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 0c88d84de6..c9ade9b517 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -112,12 +112,20 @@  static coroutine_fn void nbd_read_reply_entry(void *opaque)
     s->read_reply_co = NULL;
 }
 
-static int nbd_co_send_request(BlockDriverState *bs,
-                               NBDRequest *request,
-                               QEMUIOVector *qiov)
+static void nbd_co_receive_reply(NBDClientSession *s,
+                                 NBDRequest *request,
+                                 NBDReply *reply,
+                                 QEMUIOVector *qiov);
+static void nbd_coroutine_end(BlockDriverState *bs,
+                              NBDRequest *request);
+
+static int nbd_co_request(BlockDriverState *bs,
+                          NBDRequest *request,
+                          QEMUIOVector *qiov)
 {
     NBDClientSession *s = nbd_get_client_session(bs);
     int rc, ret, i;
+    NBDReply reply;
 
     qemu_co_mutex_lock(&s->send_mutex);
     while (s->in_flight == MAX_NBD_REQUESTS) {
@@ -141,7 +149,8 @@  static int nbd_co_send_request(BlockDriverState *bs,
         return -EPIPE;
     }
 
-    if (qiov) {
+    if (request->type == NBD_CMD_WRITE) {
+        assert(qiov != NULL);
         qio_channel_set_cork(s->ioc, true);
         rc = nbd_send_request(s->ioc, request);
         if (rc >= 0) {
@@ -156,6 +165,21 @@  static int nbd_co_send_request(BlockDriverState *bs,
         rc = nbd_send_request(s->ioc, request);
     }
     qemu_co_mutex_unlock(&s->send_mutex);
+
+    if (rc < 0) {
+        goto out;
+    }
+
+    if (request->type == NBD_CMD_READ) {
+        assert(qiov != NULL);
+    } else {
+        qiov = NULL;
+    }
+    nbd_co_receive_reply(s, request, &reply, qiov);
+    rc = -reply.error;
+
+out:
+    nbd_coroutine_end(bs, request);
     return rc;
 }
 
@@ -208,26 +232,16 @@  static void nbd_coroutine_end(BlockDriverState *bs,
 int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
                          uint64_t bytes, QEMUIOVector *qiov, int flags)
 {
-    NBDClientSession *client = nbd_get_client_session(bs);
     NBDRequest request = {
         .type = NBD_CMD_READ,
         .from = offset,
         .len = bytes,
     };
-    NBDReply reply;
-    int ret;
 
     assert(bytes <= NBD_MAX_BUFFER_SIZE);
     assert(!flags);
 
-    ret = nbd_co_send_request(bs, &request, NULL);
-    if (ret < 0) {
-        reply.error = -ret;
-    } else {
-        nbd_co_receive_reply(client, &request, &reply, qiov);
-    }
-    nbd_coroutine_end(bs, &request);
-    return -reply.error;
+    return nbd_co_request(bs, &request, qiov);
 }
 
 int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
@@ -239,8 +253,6 @@  int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
         .from = offset,
         .len = bytes,
     };
-    NBDReply reply;
-    int ret;
 
     if (flags & BDRV_REQ_FUA) {
         assert(client->info.flags & NBD_FLAG_SEND_FUA);
@@ -249,27 +261,18 @@  int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
 
     assert(bytes <= NBD_MAX_BUFFER_SIZE);
 
-    ret = nbd_co_send_request(bs, &request, qiov);
-    if (ret < 0) {
-        reply.error = -ret;
-    } else {
-        nbd_co_receive_reply(client, &request, &reply, NULL);
-    }
-    nbd_coroutine_end(bs, &request);
-    return -reply.error;
+    return nbd_co_request(bs, &request, qiov);
 }
 
 int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
                                 int bytes, BdrvRequestFlags flags)
 {
-    int ret;
     NBDClientSession *client = nbd_get_client_session(bs);
     NBDRequest request = {
         .type = NBD_CMD_WRITE_ZEROES,
         .from = offset,
         .len = bytes,
     };
-    NBDReply reply;
 
     if (!(client->info.flags & NBD_FLAG_SEND_WRITE_ZEROES)) {
         return -ENOTSUP;
@@ -283,22 +286,13 @@  int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
         request.flags |= NBD_CMD_FLAG_NO_HOLE;
     }
 
-    ret = nbd_co_send_request(bs, &request, NULL);
-    if (ret < 0) {
-        reply.error = -ret;
-    } else {
-        nbd_co_receive_reply(client, &request, &reply, NULL);
-    }
-    nbd_coroutine_end(bs, &request);
-    return -reply.error;
+    return nbd_co_request(bs, &request, NULL);
 }
 
 int nbd_client_co_flush(BlockDriverState *bs)
 {
     NBDClientSession *client = nbd_get_client_session(bs);
     NBDRequest request = { .type = NBD_CMD_FLUSH };
-    NBDReply reply;
-    int ret;
 
     if (!(client->info.flags & NBD_FLAG_SEND_FLUSH)) {
         return 0;
@@ -307,14 +301,7 @@  int nbd_client_co_flush(BlockDriverState *bs)
     request.from = 0;
     request.len = 0;
 
-    ret = nbd_co_send_request(bs, &request, NULL);
-    if (ret < 0) {
-        reply.error = -ret;
-    } else {
-        nbd_co_receive_reply(client, &request, &reply, NULL);
-    }
-    nbd_coroutine_end(bs, &request);
-    return -reply.error;
+    return nbd_co_request(bs, &request, NULL);
 }
 
 int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
@@ -325,22 +312,12 @@  int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
         .from = offset,
         .len = bytes,
     };
-    NBDReply reply;
-    int ret;
 
     if (!(client->info.flags & NBD_FLAG_SEND_TRIM)) {
         return 0;
     }
 
-    ret = nbd_co_send_request(bs, &request, NULL);
-    if (ret < 0) {
-        reply.error = -ret;
-    } else {
-        nbd_co_receive_reply(client, &request, &reply, NULL);
-    }
-    nbd_coroutine_end(bs, &request);
-    return -reply.error;
-
+    return nbd_co_request(bs, &request, NULL);
 }
 
 void nbd_client_detach_aio_context(BlockDriverState *bs)