Re: [Qemu-devel] [PATCH 07/17] block/nbd-client: refactor request send/receive

2017-08-25 Thread Eric Blake
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 
> 

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



signature.asc
Description: OpenPGP digital signature


Re: [Qemu-devel] [PATCH 07/17] block/nbd-client: refactor request send/receive

2017-08-25 Thread Eric Blake
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 
> ---
>  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 

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



signature.asc
Description: OpenPGP digital signature


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

2017-08-04 Thread Vladimir Sementsov-Ogievskiy
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 
---
 block/nbd-client.c | 89 --
 1 file changed, 33 insertions(+), 56 deletions(-)

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(>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(>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, , 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, , NULL);
-if (ret < 0) {
-reply.error = -ret;
-} else {
-nbd_co_receive_reply(client, , , qiov);
-}
-nbd_coroutine_end(bs, );
-return -reply.error;
+return nbd_co_request(bs, , 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, , qiov);
-if (ret < 0) {
-reply.error = -ret;
-} else {
-nbd_co_receive_reply(client, , , NULL);
-}
-nbd_coroutine_end(bs, );
-return -reply.error;
+return nbd_co_request(bs, , 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, , NULL);
-if (ret < 0) {
-reply.error = -ret;
-} else {
-nbd_co_receive_reply(client, , , NULL);
-}
-nbd_coroutine_end(bs, );
-return -reply.error;
+return nbd_co_request(bs, , 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