1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v2 00/12] nbd refactoring part 1
  4. View patch

[Qemu-devel] [PATCH v2 12/12] nbd/server: refactor nbd_trip

Vladimir Sementsov-Ogievskiy posted 12 patches 8 years, 8 months ago
[Qemu-devel] [PATCH v2 12/12] nbd/server: refactor nbd_trip
Posted by Vladimir Sementsov-Ogievskiy 8 years, 8 months ago 
- do not use 'goto error_reply' outside a switch to jump into the
  middle of the switch's default case label
- reduce code duplication

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

diff --git a/nbd/server.c b/nbd/server.c
index 209a3d4e1e..198eabe338 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1078,6 +1078,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     NBDReply reply;
     int ret;
     int flags;
+    int reply_data_len = 0;
 
     TRACE("Reading request.");
     if (client->closing) {
@@ -1090,7 +1091,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     client->recv_coroutine = NULL;
     nbd_client_receive_next_request(client);
     if (ret == -EIO) {
-        goto out;
+        goto disconnect;
     }
 
     reply.handle = request.handle;
@@ -1098,7 +1099,7 @@ static coroutine_fn void nbd_trip(void *opaque)
 
     if (ret < 0) {
         reply.error = -ret;
-        goto error_reply;
+        goto reply;
     }
 
     if (client->closing) {
@@ -1119,7 +1120,7 @@ static coroutine_fn void nbd_trip(void *opaque)
             if (ret < 0) {
                 LOG("flush failed");
                 reply.error = -ret;
-                goto error_reply;
+                break;
             }
         }
 
@@ -1128,12 +1129,12 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (ret < 0) {
             LOG("reading from file failed");
             reply.error = -ret;
-            goto error_reply;
+            break;
         }
 
+        reply_data_len = request.len;
         TRACE("Read %" PRIu32" byte(s)", request.len);
-        if (nbd_co_send_reply(req, &reply, request.len) < 0)
-            goto out;
+
         break;
     case NBD_CMD_WRITE:
         TRACE("Request type is WRITE");
@@ -1141,7 +1142,7 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
             TRACE("Server is read-only, return error");
             reply.error = EROFS;
-            goto error_reply;
+            break;
         }
 
         TRACE("Writing to device");
@@ -1155,21 +1156,16 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (ret < 0) {
             LOG("writing to file failed");
             reply.error = -ret;
-            goto error_reply;
         }
 
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
         break;
-
     case NBD_CMD_WRITE_ZEROES:
         TRACE("Request type is WRITE_ZEROES");
 
         if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
             TRACE("Server is read-only, return error");
             reply.error = EROFS;
-            goto error_reply;
+            break;
         }
 
         TRACE("Writing to device");
@@ -1186,14 +1182,9 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (ret < 0) {
             LOG("writing to file failed");
             reply.error = -ret;
-            goto error_reply;
         }
 
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
         break;
-
     case NBD_CMD_DISC:
         /* unreachable, thanks to special case in nbd_co_receive_request() */
         abort();
@@ -1206,9 +1197,7 @@ static coroutine_fn void nbd_trip(void *opaque)
             LOG("flush failed");
             reply.error = -ret;
         }
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
+
         break;
     case NBD_CMD_TRIM:
         TRACE("Request type is TRIM");
@@ -1218,21 +1207,19 @@ static coroutine_fn void nbd_trip(void *opaque)
             LOG("discard failed");
             reply.error = -ret;
         }
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
+
         break;
     default:
         LOG("invalid request type (%" PRIu32 ") received", request.type);
         reply.error = EINVAL;
-    error_reply:
-        /* We must disconnect after NBD_CMD_WRITE if we did not
-         * read the payload.
-         */
-        if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete) {
-            goto out;
-        }
-        break;
+    }
+
+reply:
+    /* We must disconnect after NBD_CMD_WRITE if we did not
+     * read the payload.
+     */
+    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0 || !req->complete) {
+        goto disconnect;
     }
 
     TRACE("Request/Reply complete");
@@ -1242,7 +1229,7 @@ done:
     nbd_client_put(client);
     return;
 
-out:
+disconnect:
     nbd_request_put(req);
     client_close(client);
     nbd_client_put(client);
-- 
2.11.1
Re: [Qemu-devel] [PATCH v2 12/12] nbd/server: refactor nbd_trip
Posted by Paolo Bonzini 8 years, 8 months ago 

On 02/06/2017 17:01, Vladimir Sementsov-Ogievskiy wrote:
> - do not use 'goto error_reply' outside a switch to jump into the
>   middle of the switch's default case label
> - reduce code duplication
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

As a followup, maybe the switch statement can be extracted to its own
function.  This way the "goto reply" can be avoided.  I'll take a look.

Apart from this, the series looks good.  I'll queue it tomorrow.

Paolo


> ---
>  nbd/server.c | 53 ++++++++++++++++++++---------------------------------
>  1 file changed, 20 insertions(+), 33 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 209a3d4e1e..198eabe338 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1078,6 +1078,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>      NBDReply reply;
>      int ret;
>      int flags;
> +    int reply_data_len = 0;
>  
>      TRACE("Reading request.");
>      if (client->closing) {
> @@ -1090,7 +1091,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>      client->recv_coroutine = NULL;
>      nbd_client_receive_next_request(client);
>      if (ret == -EIO) {
> -        goto out;
> +        goto disconnect;
>      }
>  
>      reply.handle = request.handle;
> @@ -1098,7 +1099,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>  
>      if (ret < 0) {
>          reply.error = -ret;
> -        goto error_reply;
> +        goto reply;
>      }
>  
>      if (client->closing) {
> @@ -1119,7 +1120,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>              if (ret < 0) {
>                  LOG("flush failed");
>                  reply.error = -ret;
> -                goto error_reply;
> +                break;
>              }
>          }
>  
> @@ -1128,12 +1129,12 @@ static coroutine_fn void nbd_trip(void *opaque)
>          if (ret < 0) {
>              LOG("reading from file failed");
>              reply.error = -ret;
> -            goto error_reply;
> +            break;
>          }
>  
> +        reply_data_len = request.len;
>          TRACE("Read %" PRIu32" byte(s)", request.len);
> -        if (nbd_co_send_reply(req, &reply, request.len) < 0)
> -            goto out;
> +
>          break;
>      case NBD_CMD_WRITE:
>          TRACE("Request type is WRITE");
> @@ -1141,7 +1142,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>          if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
>              TRACE("Server is read-only, return error");
>              reply.error = EROFS;
> -            goto error_reply;
> +            break;
>          }
>  
>          TRACE("Writing to device");
> @@ -1155,21 +1156,16 @@ static coroutine_fn void nbd_trip(void *opaque)
>          if (ret < 0) {
>              LOG("writing to file failed");
>              reply.error = -ret;
> -            goto error_reply;
>          }
>  
> -        if (nbd_co_send_reply(req, &reply, 0) < 0) {
> -            goto out;
> -        }
>          break;
> -
>      case NBD_CMD_WRITE_ZEROES:
>          TRACE("Request type is WRITE_ZEROES");
>  
>          if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
>              TRACE("Server is read-only, return error");
>              reply.error = EROFS;
> -            goto error_reply;
> +            break;
>          }
>  
>          TRACE("Writing to device");
> @@ -1186,14 +1182,9 @@ static coroutine_fn void nbd_trip(void *opaque)
>          if (ret < 0) {
>              LOG("writing to file failed");
>              reply.error = -ret;
> -            goto error_reply;
>          }
>  
> -        if (nbd_co_send_reply(req, &reply, 0) < 0) {
> -            goto out;
> -        }
>          break;
> -
>      case NBD_CMD_DISC:
>          /* unreachable, thanks to special case in nbd_co_receive_request() */
>          abort();
> @@ -1206,9 +1197,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>              LOG("flush failed");
>              reply.error = -ret;
>          }
> -        if (nbd_co_send_reply(req, &reply, 0) < 0) {
> -            goto out;
> -        }
> +
>          break;
>      case NBD_CMD_TRIM:
>          TRACE("Request type is TRIM");
> @@ -1218,21 +1207,19 @@ static coroutine_fn void nbd_trip(void *opaque)
>              LOG("discard failed");
>              reply.error = -ret;
>          }
> -        if (nbd_co_send_reply(req, &reply, 0) < 0) {
> -            goto out;
> -        }
> +
>          break;
>      default:
>          LOG("invalid request type (%" PRIu32 ") received", request.type);
>          reply.error = EINVAL;
> -    error_reply:
> -        /* We must disconnect after NBD_CMD_WRITE if we did not
> -         * read the payload.
> -         */
> -        if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete) {
> -            goto out;
> -        }
> -        break;
> +    }
> +
> +reply:
> +    /* We must disconnect after NBD_CMD_WRITE if we did not
> +     * read the payload.
> +     */
> +    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0 || !req->complete) {
> +        goto disconnect;
>      }
>  
>      TRACE("Request/Reply complete");
> @@ -1242,7 +1229,7 @@ done:
>      nbd_client_put(client);
>      return;
>  
> -out:
> +disconnect:
>      nbd_request_put(req);
>      client_close(client);
>      nbd_client_put(client);
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.