Re: [Qemu-devel] [PATCH 13/19] nbd/server: return original error codes

[Vladimir Sementsov-Ogievskiy]

02.06.2017 01:29, Eric Blake wrote:
> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> > The code in many cases return -EINVAL or -EIO instead of original error
> > code from, for example, write_sync(). Following patch will need EPIPE
> > handling, so, let's refactor this where possible (the only exclusion
> > is nbd_co_receive_request, with own return-code convention)
> Do we still want/need EPIPE handling, given the discussion on the
> previous two patches?

Looks like this patch should be dropped. If EPIPE is not accepted 
(previous discussion), so error code is always EIO, no needs to save it 
into a variable..

> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   nbd/server.c | 124 +++++++++++++++++++++++++++++++++++++----------------------
> >   1 file changed, 77 insertions(+), 47 deletions(-)
> Feels weird to have a net gain in code, but maybe worthwhile.
>
> > diff --git a/nbd/server.c b/nbd/server.c
> > index a47f13e4fb..30dfb81a5c 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -136,30 +136,38 @@ static void nbd_client_receive_next_request(NBDClient 
> > *client);
> >   static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
> >                                         uint32_t opt, uint32_t len)
> >   {
> > +    int ret;
> >       uint64_t magic;
> >   
> >       TRACE("Reply opt=%" PRIx32 " type=%" PRIx32 " len=%" PRIu32,
> >             type, opt, len);
> >   
> >       magic = cpu_to_be64(NBD_REP_MAGIC);
> > -    if (write_sync(ioc, &magic, sizeof(magic), NULL) < 0) {
> > +    ret = write_sync(ioc, &magic, sizeof(magic), NULL);
> > +    if (ret < 0) {
> >           LOG("write failed (rep magic)");
> > -        return -EINVAL;
> > +        return ret;
> >       }
> Constructs like this should get shorter once we plumb errp all the way
> through.  Okay, I can live with the temporary verbosity.
>
> You may still have to make changes due to rebasing (in which case I'll
> definitely want to review again); but if this patch doesn't need further
> rework, you can add:
> Reviewed-by: Eric Blake <address@hidden>

--
Best regards,
Vladimir