Re: [Qemu-devel] [PATCH] Fix NBD unsupported options

[Eric Blake]

On 04/05/2016 12:55 PM, 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)

I just re-reviewed this patch. While what you have is a strict
improvement, it is still buggy for a server that sends a UTF-8 message
explaining the error:

> @@ -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;
> +    }

That is, if len > 0, we are still leaving the server's UTF-8 message on
the wire, but returning early, which will still corrupt the next attempt
to send another option.

Paolo, can you revert this off your queue, and wait for me to send a v2
that fixes the situation in a cleaner manner?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature