[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Fix NBD unsupported options
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] Fix NBD unsupported options |
Date: |
Wed, 6 Apr 2016 10:06:55 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
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 <address@hidden>
> ---
> 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