[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v6 06/12] nbd/server: Refactor zero-length optio
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v6 06/12] nbd/server: Refactor zero-length option check |
Date: |
Mon, 30 Oct 2017 21:11:53 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
On 10/30/2017 06:22 PM, Vladimir Sementsov-Ogievskiy wrote:
> 27.10.2017 13:40, Eric Blake wrote:
>> Consolidate the response for a non-zero-length option payload
>> into a new function, nbd_reject_length(). This check will
>> also be used when introducing support for structured replies.
>>
>> Note that STARTTLS response differs based on time: if the connection
>> is still unencrypted, we set fatal to true (a client that can't
>> request TLS correctly may still think that we are ready to start
>> the TLS handshake, so we must disconnect); while if the connection
>> is already encrypted, the client is sending a bogus request but
>> is no longer at risk of being confused by continuing the connection.
>>
>> switch (option) {
>> case NBD_OPT_STARTTLS:
>> - tioc = nbd_negotiate_handle_starttls(client, length,
>> errp);
>> + if (length) {
>> + /* Unconditionally drop the connection if the client
>> + * can't start a TLS negotiation correctly */
>> + nbd_reject_length(client, length, option, true,
>> errp);
>> + return -EINVAL;
>
> why not to return nbd_reject_length's result? this EINVAL may not
> correspond to errp (about EIO for example)..
Somewhat true, if nbd_reject_length() fails. But nbd_reject_length() may
also return 0 without setting errp, in which case, maybe this code
should set errp rather than just blindly returning -EINVAL.
>
> with or without this fixed:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>
Okay, I'll squash this in, and include it in my pull request to be sent
shortly.
diff --git i/nbd/server.c w/nbd/server.c
index a9480e42cd..91f81a0f19 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -684,8 +684,13 @@ static int nbd_negotiate_options(NBDClient *client,
uint16_t myflags,
if (length) {
/* Unconditionally drop the connection if the client
* can't start a TLS negotiation correctly */
- nbd_reject_length(client, length, option, true, errp);
- return -EINVAL;
+ ret = nbd_reject_length(client, length, option,
true, errp);
+ if (!ret) {
+ error_setg(errp, "option '%s' should have zero
length",
+ nbd_opt_lookup(option));
+ ret = -EINVAL;
+ }
+ return ret;
}
tioc = nbd_negotiate_handle_starttls(client, errp);
if (!tioc) {
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v6 00/12] nbd minimal structured read, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 01/12] nbd: Include error names in trace messages, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 02/12] nbd: Move nbd_errno_to_system_errno() to public header, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 03/12] nbd: Expose constants and structs for structured read, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 04/12] nbd/server: Report error for write to read-only export, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 05/12] nbd/server: Simplify nbd_negotiate_options loop, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 06/12] nbd/server: Refactor zero-length option check, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 07/12] nbd: Minimal structured read for server, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 08/12] nbd/server: Include human-readable message in structured errors, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 10/12] nbd/client: prepare nbd_receive_reply for structured reply, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 09/12] nbd/client: refactor nbd_receive_starttls, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 11/12] nbd: Move nbd_read() to common header, Eric Blake, 2017/10/27
- [Qemu-block] [PATCH v6 12/12] nbd: Minimal structured read for client, Eric Blake, 2017/10/27