Re: [Qemu-devel] [PATCH 1/2] nbd: Handle fixed new-style clients.

[Hani Benhabiles]

On Tue, May 27, 2014 at 04:46:46PM +0200, Paolo Bonzini wrote:
> Il 25/05/2014 11:50, Hani Benhabiles ha scritto:
> >@@ -236,9 +236,10 @@ static int nbd_receive_options(NBDClient *client)
> >         LOG("read failed");
> >         goto fail;
> >     }
> >-    TRACE("Checking reserved");
> >-    if (tmp != 0) {
> >-        LOG("Bad reserved received");
> >+    TRACE("Checking client flags");
> >+    tmp = be32_to_cpu(tmp);
> >+    if (tmp != 0 && tmp != NBD_FLAG_C_FIXED_NEWSTYLE) {
> >+        LOG("Bad client flags received");
> >         goto fail;
> >     }
> >
> >@@ -246,7 +247,7 @@ static int nbd_receive_options(NBDClient *client)
> >         LOG("read failed");
> >         goto fail;
> >     }
> >-    TRACE("Checking reserved");
> >+    TRACE("Checking opts magic");
> >     if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
> >         LOG("Bad magic received");
> >         goto fail;
> 
> Here, if the client used "fixed new-style negotiation" you should reply with
> NBD_REP_ERR_UNSUP.  You also need to turn this into a while loop. With these
> changes here, the logic in patch 2 should be okay.

Hmm, seems that both the nbd-server implementation and the accompanying 
documentation
use NBD_REP_ERR_UNSUP only when the NBD_OPT_* asked for by the client is
unknown, not when the magic value is not NBD_OPTS_MAGIC. So what you are
suggesting should actually happen a little below in the code.

Thanks for the review. Will respin to add that and the loop behaviour.