Re: [Qemu-devel] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client

[Vladimir Sementsov-Ogievskiy]

19.07.2017 20:12, Eric Blake wrote:
> On 07/19/2017 11:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> > 07.07.2017 23:30, Eric Blake wrote:
> >
> > [..]
> >
> > > +/* Returns -1 if NBD_OPT_GO proves the export @wantname cannot be
> > > + * used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
> > > + * NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
> > > + * go (with @info populated). */
> > > +static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
> > > +                      NBDExportInfo *info, Error **errp)
> > > +{
> > > +        if (reply.type == NBD_REP_ACK) {
> > > +            /* Server is done sending info and moved into transmission
> > > +               phase, but make sure it sent flags */
> > > +            if (len) {
> > > +                error_setg(errp, "server sent invalid NBD_REP_ACK");
> > > +                nbd_send_opt_abort(ioc);
> > server is already in transmission phase, so we cant send any options
> > more, shouldn't CMD_DISC be send here instead?
> >
> > > +                return -1;
> > > +            }
> > > +            if (!info->flags) {
> > > +                error_setg(errp, "broken server omitted
> > > NBD_INFO_EXPORT");
> > > +                nbd_send_opt_abort(ioc);
> > and here
> For NBD_OPT_INFO, receipt of NBD_REP_ACK implies the server is still in

hmm, nothing here about NBD_OPT_INFO.

> negotiation phase; transmission phases is only technically possible on
> NBD_OPT_GO.  That said, either error condition means the server is
> buggy, so it really doesn't matter what we do in response (we don't know
> if the server moved on to transmission phase or not, because it has
> already broken protocol by sending us garbage) - so trying to be
> courteous to tell the server we don't like their garbage vs. just
> silently disconnecting makes no real difference; even if the server
> doesn't like what we send (because it thinks we are out of phase), the
> server already broke protocol first.  Either way, we're going to
> disconnect, and the server has to mop up after its own bugs.

agree

> I don't think a followup patch is essential, but I'd also be okay with a
> patch that just eliminates the NBD_OPT_ABORT attempt and does a silent
> disconnect instead.

--
Best regards,
Vladimir