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 <vsement...@virtuozzo.com> 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