Re: [Qemu-devel] [PATCH] nbd/server: fix space read

[Eric Blake <eblake@redhat.com>]

On 03/05/2018 12:04 PM, Vladimir Sementsov-Ogievskiy wrote:
> In case of io error in nbd_co_send_sparse_read we should not
> "goto reply:", as it is fatal error and common behavior is
> disconnect in this case. We should not try to send client an
> error reply, representing channel-io error on previous try to
> send a reply.

Good catch.

> 
> Fix this by handle block-status error in nbd_co_send_sparse_read,
> so nbd_co_send_sparse_read fails only on io error. Then just skip
> common "reply:" code path in nbd_trip.
> 
> Note: nbd_co_send_structured_error is moved without changes to be
> called from nbd_co_send_sparse_read.

Might be easier to read as two patches, one for the code motion, the 
other for using the new code.  But I'm not going to insist on a split.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> PS: this all shows that nbd_trip is tooo complex (and yes, I remember,
> that its final semantics was made by myself :(.. It should be
> refactored into several smaller parts. Do you have any ideas?
> 
> The complexity here is that we should handle channel errors (fatal) and
> export errors(non fatal) in different ways, and both of error types may
> have errp, which should be handled differently too..
> 
> May be, the best way is to make separate functions for each command,
> avoiding code duplication by using helper-functions instead of common code
> in nbd_trip.

Yes, splitting nbd_trip into smaller helper functions may be worthwhile.

> 
>   nbd/server.c | 64 +++++++++++++++++++++++++++++++++++-------------------------
>   1 file changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 4990a5826e..ea6b9467e4 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -21,6 +21,7 @@
>   #include "qapi/error.h"
>   #include "trace.h"
>   #include "nbd-internal.h"
> +#include "qemu/error-report.h"

I'm a bit worried about this one.  The server has previously not needed 
this, so it may be the wrong thing to pull it in now.


> +
>   static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
>                                                   uint64_t handle,
>                                                   uint64_t offset,

It doesn't help that we are lacking comments on the contract of this 
function.

> @@ -1361,8 +1386,15 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
>           bool final;
>   
>           if (status < 0) {
> -            error_setg_errno(errp, -status, "unable to check for holes");
> -            return status;
> +            char *msg = g_strdup_printf("unable to check for holes: %s",
> +                                              strerror(-status));

Indentation looks off.

> +
> +            error_report("%s", msg);

Do we really need to report this on the server's stderr, or is sending 
the reply to the client good enough?

> +
> +            ret = nbd_co_send_structured_error(client, handle, -status, msg,
> +                                               errp);
> +            g_free(msg);
> +            return ret;

So if we have an early return here, then pre-patch, we were 
unconditionally setting errp and returning -1 (even if the client is 
still alive); post-patch errp is only set if we failed to do I/O with 
the client.  That change is right, but harder to see without comments 
giving a function contract.

> @@ -1567,7 +1575,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>                                             request.from, req->data, request.len,
>                                             &local_err);
>               if (ret < 0) {
> -                goto reply;
> +                goto replied;
>               }
>               goto done;
>           }
> @@ -1664,6 +1672,8 @@ reply:
>                                          req->data, reply_data_len, &local_err);
>       }
>       g_free(msg);
> +
> +replied:

This part makes sense.

>       if (ret < 0) {
>           error_prepend(&local_err, "Failed to send reply: ");
>           goto disconnect;
> 

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