Re: [Qemu-devel] [PATCH 14/19] nbd/server: nbd_negotiate: return 1 on NBD_OPT_ABORT

[Vladimir Sementsov-Ogievskiy]

02.06.2017 15:55, Vladimir Sementsov-Ogievskiy wrote:
> 02.06.2017 01:33, Eric Blake wrote:
> > On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > Separate case when client sent NBD_OPT_ABORT from other errors.
> > > It will be needed for the following patch, where errors will be
> > > reported.
> > > Considered case is not actually the error - it honestly follows NBD
> > > protocol. Therefore it should not be reported like an error.
> > > -EPIPE case means client not read server reply on NBD_OPT_ABORT,
> > > which is also OK.
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > > ---
> > >   nbd/server.c | 20 +++++++++++++++-----
> > >   1 file changed, 15 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/nbd/server.c b/nbd/server.c
> > > index 30dfb81a5c..0e53d3dd91 100644
> > > --- a/nbd/server.c
> > > +++ b/nbd/server.c
> > > @@ -369,9 +369,13 @@ static QIOChannel 
> > > *nbd_negotiate_handle_starttls(NBDClient *client,
> > >       return QIO_CHANNEL(tioc);
> > >   }
> > >   -
> > > -/* Process all NBD_OPT_* client option commands.
> > > - * Return -errno on error, 0 on success. */
> > > +/* nbd_negotiate_options
> > > + * Process all NBD_OPT_* client option commands.
> > > + * Return:
> > > + * < 0 on error
> > Do you want to be specific that this is a negative errno value, or is it
> > just any negative value with no correlation to errno?
>
> nothing here (except blk_write and friends, but their errors are not 
> returned by any function) correlates with errno, because core function 
> - nbd_wr_syncv returns EIO on any error. Underlying io_channel returns 
> -1 or QIO_CHANNEL_ERR_BLOCK...

changed to "-errno  on error"

> > > + * 0   on successful negotiation
> > > + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
> > > + */
> > >   static int nbd_negotiate_options(NBDClient *client)
> > >   {
> > >       int ret;
> > > @@ -483,7 +487,7 @@ static int nbd_negotiate_options(NBDClient *client)
> > >                   }
> > >                   /* Let the client keep trying, unless they asked 
> > > to quit */
> > >                   if (clientflags == NBD_OPT_ABORT) {
> > > -                    return -EINVAL;
> > > +                    return 1;
> > >                   }
> > >                   break;
> > >               }
> > > @@ -502,7 +506,7 @@ static int nbd_negotiate_options(NBDClient *client)
> > >                    * guests that don't wait for our reply. */
> > >                   ret = nbd_negotiate_send_rep(client->ioc, 
> > > NBD_REP_ACK,
> > >                                                clientflags);
> > > -                return ret < 0 ? ret : -EINVAL;
> > > +                return ret < 0 && ret != -EPIPE ? ret : 1;
> > This should just be 'return 1;', which means you don't need to capture
> > and check 'ret'.
> >
> > >                 case NBD_OPT_EXPORT_NAME:
> > >                   return nbd_negotiate_handle_export_name(client, 
> > > length);
> > > @@ -560,6 +564,12 @@ static int nbd_negotiate_options(NBDClient 
> > > *client)
> > >       }
> > >   }
> > >   +/* nbd_negotiate
> > > + * Return:
> > > + * < 0 on error
> > Again, if this is reliably a negative errno, specifically document that.
> >
> > > + * 0   on successful negotiation
> > > + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
> > > + */
> > >   static coroutine_fn int nbd_negotiate(NBDClient *client)
> > >   {
> > >       char buf[8 + 8 + 8 + 128];

--
Best regards,
Vladimir