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 <ebl...@redhat.com> > > 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
