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

[Vladimir Sementsov-Ogievskiy]

08.03.2018 18:17, Vladimir Sementsov-Ogievskiy wrote:
> 08.03.2018 14:50, Vladimir Sementsov-Ogievskiy wrote:
> > 05.03.2018 22:47, Eric Blake wrote:
> > > 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 <address@hidden>
> > > > ---
> > > >
> > > > 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?
> >
> >
> > I'm just trying to save current behavior in nbd_trip... Why not? Is 
> > it fair that client knows about some strange io error on server-side, 
> > but server logs lacks this information?
>
> May be, it worth switching it from error_report to trace, but it 
> should be a separate patch, changing all places around.

stop, it is already traced in nbd_co_send_simple_reply and 
nbd_co_send_structured_error. So, we can safely drop it.


> > and it needs "qemu/error-report.h".. nbd_trip uses error_report_err 
> > instead. error_report_err calls error_report anyway.
> >
> > > > +
> > > > +            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;


--
Best regards,
Vladimir