From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35220) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aniUU-0005dF-JL for qemu-devel@nongnu.org; Wed, 06 Apr 2016 04:07:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aniUQ-0007h8-0x for qemu-devel@nongnu.org; Wed, 06 Apr 2016 04:07:06 -0400 Received: from mail-wm0-x243.google.com ([2a00:1450:400c:c09::243]:34689) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aniUP-0007h4-Mc for qemu-devel@nongnu.org; Wed, 06 Apr 2016 04:07:01 -0400 Received: by mail-wm0-x243.google.com with SMTP id n3so10859040wmn.1 for ; Wed, 06 Apr 2016 01:07:01 -0700 (PDT) Sender: Paolo Bonzini References: <1459882500-24316-1-git-send-email-alex@alex.org.uk> From: Paolo Bonzini Message-ID: <5704C39F.5070002@redhat.com> Date: Wed, 6 Apr 2016 10:06:55 +0200 MIME-Version: 1.0 In-Reply-To: <1459882500-24316-1-git-send-email-alex@alex.org.uk> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Fix NBD unsupported options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Bligh , "Daniel P. Berrange" , Kevin Wolf , Eric Blake , "qemu-devel@nongnu.org" Cc: Wouter Verhelst On 05/04/2016 20:55, Alex Bligh wrote: > nbd-client.c currently fails to handle unsupported options properly. > If during option haggling the server finds an option that is > unsupported, it returns an NBD_REP_ERR_UNSUP reply. > > According to nbd's proto.md, the format for such a reply > should be: > > S: 64 bits, 0x3e889045565a9 (magic number for replies) > S: 32 bits, the option as sent by the client to which this is a reply > S: 32 bits, reply type (e.g., NBD_REP_ACK for successful completion, > or NBD_REP_ERR_UNSUP to mark use of an option not known by this server > S: 32 bits, length of the reply. This may be zero for some replies, > in which case the next field is not sent > S: any data as required by the reply (e.g., an export name in the case > of NBD_REP_SERVER) > > However, in nbd-client.c, the reply type was being read, and if it > contained an error, it was bailing out and issuing the next option > request without first reading the length. This meant that the > next option / handshake read had an extra 4 bytes of data in it. > In practice, this makes Qemu incompatible with servers that do not > support NBD_OPT_LIST. > > To verify this isn't an error in the specification or my reading of > it, replies are sent by the reference implementation here: > https://github.com/yoe/nbd/blob/master/nbd-server.c#L1232 > and as is evident it always sends a 'datasize' (aka length) 32 bit > word. Unsupported elements are replied to here: > https://github.com/yoe/nbd/blob/master/nbd-server.c#L1371 > > Signed-off-by: Alex Bligh > --- > nbd/client.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/nbd/client.c b/nbd/client.c > index d9b7a9b..6f0541d 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -138,12 +138,6 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp) > return -1; > } > type = be32_to_cpu(type); > - if (type == NBD_REP_ERR_UNSUP) { > - return 0; > - } > - if (nbd_handle_reply_err(opt, type, errp) < 0) { > - return -1; > - } > > if (read_sync(ioc, &len, sizeof(len)) != sizeof(len)) { > error_setg(errp, "failed to read option length"); > @@ -151,6 +145,13 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp) > } > len = be32_to_cpu(len); > > + if (type == NBD_REP_ERR_UNSUP) { > + return 0; > + } > + if (nbd_handle_reply_err(opt, type, errp) < 0) { > + return -1; > + } > + > if (type == NBD_REP_ACK) { > if (len != 0) { > error_setg(errp, "length too long for option end"); > Thanks, queued. Paolo