[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v5 05/11] nbd/server: Refactor zero-length optio
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v5 05/11] nbd/server: Refactor zero-length option check |
Date: |
Fri, 20 Oct 2017 10:07:21 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 10/20/2017 03:34 AM, Vladimir Sementsov-Ogievskiy wrote:
> 20.10.2017 01:26, Eric Blake wrote:
>> Consolidate the check for a zero-length payload to an option
>> into a new function, nbd_check_zero_length(); this check will
>> also be used when introducing support for structured replies.
>>
>> By sticking a catch-all check at the end of the loop for
>> processing options, we can simplify several of the intermediate
>> cases.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>
> looks like two patches in one, however I'm not against (considering my
> big patches =)).
> I've already put an r-b here but suddenly understood a hidden behavior
> change you've made,
> which may considered like a bug, see below.
>
>> +/* nbd_check_zero_length: Handle any unexpected payload.
>> + * Return:
>> + * -errno on error, errp is set
>> + * 0 on successful negotiation, errp is not set
>> + */
>> +static int nbd_check_zero_length(NBDClient *client, uint32_t length,
>> + uint32_t option, Error **errp)
>> +{
>> + if (!length) {
>> + return 0;
>> + }
>> + if (nbd_drop(client->ioc, length, errp) < 0) {
>> + return -EIO;
>> + }
>> + return nbd_negotiate_send_rep_err(client->ioc,
>> NBD_REP_ERR_INVALID, option,
>> + errp, "option %s should have
>> zero length",
>
> may be quotes around %s or your trace-notation %d (%s) would be more
> readable
quotes don't hurt, but since none of the option names contain spaces,
it's not quite as important as when you are quoting a message sent over
the wire.
>
>> + nbd_opt_lookup(option));
>> +}
>> +
>> /* nbd_negotiate_options
>> * Process all NBD_OPT_* client option commands, during fixed newstyle
>> * negotiation.
>> @@ -674,7 +672,11 @@ static int nbd_negotiate_options(NBDClient
>> *client, uint16_t myflags,
>> }
>> switch (option) {
>> case NBD_OPT_STARTTLS:
>> - tioc = nbd_negotiate_handle_starttls(client, length,
>> errp);
>> + ret = nbd_check_zero_length(client, length, option,
>> errp);
>> + if (ret < 0) {
>> + return ret;
>> + }
>
> no, you should not continue if length>0 (old behavior).
> nbd_negotiate_send_rep_err returns 0 on success
> in nbd_check_zero_length().
Oh, good catch. But it's subtler than that. In the old code,
nbd_negotiate_handle_starttls() returns NULL on non-zero length (even if
it sent a message to the client), because we really want to kill the
connection if a client can't turn on TLS correctly...
>> @@ -712,9 +711,9 @@ static int nbd_negotiate_options(NBDClient
>> *client, uint16_t myflags,
>> } else if (fixedNewstyle) {
>> switch (option) {
>> case NBD_OPT_LIST:
>> - ret = nbd_negotiate_handle_list(client, length, errp);
>> - if (ret < 0) {
>> - return ret;
>> + ret = nbd_check_zero_length(client, length, option,
>> errp);
>> + if (!ret) {
>
> the same here
>
while nbd_negotiate_handle_list() used to return 0 if the client sent
non-zero length (we handled the incorrect message from the client just
fine, and can continue listening for more options).
Maybe I can fix it with a tri-state return: 1 if correct length, 0 if
nonzero length but error message sent successfully, and negative on
transmission failure; although then it's trickier for callers. I'll
have to think about it...
>> case NBD_OPT_STARTTLS:
>> - if (nbd_drop(client->ioc, length, errp) < 0) {
>> - return -EIO;
>> - }
>> - if (client->tlscreds) {
>> + if (length) {
>> + ret = nbd_check_zero_length(client, length,
>> option, errp);
Maybe explicitly checking for length at each caller is the simplest
approach for getting the decision logic correct, since I really wasn't
able to abstract a clean "failure to communicate" vs. "error message
sent, go on to next message or abort connection as appropriate" vs.
"everything validated, proceed with rest of handing current option".
--
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 v5 00/11] nbd minimal structured read, Eric Blake, 2017/10/19
- [Qemu-block] [PATCH v5 02/11] nbd: Move nbd_errno_to_system_errno() to public header, Eric Blake, 2017/10/19
- [Qemu-block] [PATCH v5 04/11] nbd/server: Report error for write to read-only export, Eric Blake, 2017/10/19
- [Qemu-block] [PATCH v5 03/11] nbd: Expose constants and structs for structured read, Eric Blake, 2017/10/19
- [Qemu-block] [PATCH v5 01/11] nbd: Include error names in trace messages, Eric Blake, 2017/10/19
- [Qemu-block] [PATCH v5 05/11] nbd/server: Refactor zero-length option check, Eric Blake, 2017/10/19
- [Qemu-block] [PATCH v5 08/11] nbd/client: refactor nbd_receive_starttls, Eric Blake, 2017/10/19
- [Qemu-block] [PATCH v5 09/11] nbd/client: prepare nbd_receive_reply for structured reply, Eric Blake, 2017/10/19
- [Qemu-block] [PATCH v5 10/11] nbd: Move nbd_read() to common header, Eric Blake, 2017/10/19
- [Qemu-block] [PATCH v5 06/11] nbd: Minimal structured read for server, Eric Blake, 2017/10/19