[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 5/6] nbd/server: Add helper functions for par
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 5/6] nbd/server: Add helper functions for parsing option payload |
Date: |
Fri, 12 Jan 2018 08:37:45 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 |
On 01/12/2018 04:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.01.2018 02:08, Eric Blake wrote:
>> Rather than making every callsite perform length sanity checks
>> and error reporting, add the helper functions nbd_opt_read()
>> and nbd_opt_drop() that use the length stored in the client
>> struct; also add an assertion that optlen is reduced to zero
>> after each option is handled.
>>
>> Note that the call in nbd_negotiate_handle_export_name() does
>> not use the new helper (in part because the server cannot
>> reply to NBD_OPT_EXPORT_NAME - it either succeeds or the
>> connection drops).
>>
>>
>> +/* Drop remainder of the current option, after sending a reply with
>
> looks a bit weird: actually you drop the remainder _before_ sending a
> reply)
Good catch. I'll fix it with s/after/and/
>
>> + * the given error type and message. Return -errno on read or write
>
> also, unrelated note, -errno is always forced to -EIO, because of
> nbd_read realization.
> and this note applies to many other places here. It is correct (EIO is
> errno, why not?),
> but it may be not bad to note it somewhere..
Someday nbd_read() might fail with something other than EIO (ESHUTDOWN,
perhaps?), in which case leaving this documented as -errno would be
better than hardcoding that EIO is the only failure for now.
>> @@ -812,14 +819,9 @@ static int nbd_negotiate_options(NBDClient
>> *client, uint16_t myflags,
>> break;
>>
>> default:
>> - if (nbd_drop(client->ioc, length, errp) < 0) {
>> - return -EIO;
>> - }
>> - ret = nbd_negotiate_send_rep_err(client,
>> - NBD_REP_ERR_UNSUP,
>> errp,
>> - "Unsupported option
>> 0x%"
>> - PRIx32 " (%s)", option,
>> -
>> nbd_opt_lookup(option));
>> + ret = nbd_opt_drop(client, NBD_REP_ERR_UNSUP, errp,
>> + "Unsupported option 0x%" PRIx32 "
>> (%s)",
>> + option, nbd_opt_lookup(option));
>> break;
>> }
>> } else {
>> @@ -842,6 +844,7 @@ static int nbd_negotiate_options(NBDClient
>> *client, uint16_t myflags,
>> if (ret < 0) {
>> return ret;
>> }
>> + assert(!client->optlen);
>
> isn't it from 2/6?
No, this is a second instance of the assertion, the one that applies
between each option (which I couldn't do in 2/6 because not all options
were manipulating optlen back then). Maybe I can tweak the commit
messages to make that more obvious.
>
>> }
>> }
>>
>
> anyway,
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
Thanks for the careful attention to detail.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 6/6] nbd/server: structurize option reply sending, (continued)
[Qemu-devel] [PATCH v2 5/6] nbd/server: Add helper functions for parsing option payload, Eric Blake, 2018/01/10
[Qemu-devel] [PATCH v2 4/6] nbd/server: Add va_list form of nbd_negotiate_send_rep_err(), Eric Blake, 2018/01/10