On 01/12/2018 04:18 AM, Vladimir Sementsov-Ogievskiy wrote: >>> hmm, should not it be at the end of nbd_negotiate() ? Looks OK anyway. >> Sure, I'll squash this in, for no real change in behavior: >> >> diff --git i/nbd/server.c w/nbd/server.c >> index c22d5e6ecf..3fd145592e 100644 >> --- i/nbd/server.c >> +++ w/nbd/server.c >> @@ -902,6 +902,7 @@ static coroutine_fn int nbd_negotiate(NBDClient >> *client, Error **errp) >> } >> } >> >> + assert(!client->optlen); >> trace_nbd_negotiate_success(); >>
This says that after all negotiation is complete, optlen is 0 (so in reality, it is only checking NBD_OPT_GO and NBD_OPT_EXPORT_NAME - since those are the only two options that can end negotiation). But it does not check state in between individual options during the actual negotiation. Also, this is the only spot in the code that can check that optlen is 0 even for old-style connections (where we do not call nbd_negotiate_options), so it's a nice spot to prove that when we move into transmission phase, we aren't stranding any unread data from handshake phase. > > > or, even better, in nbd_negotiate_options. and you actually do it in 5/6. Whereas that is stating that optlen is 0 after every option that does not return early (not possible in this patch, because we need the nbd_opt_read() helper in 5/6 that clears optlen as we go) - and meanwhile does NOT cover the places that return early (NBD_OPT_GO and NBD_OPT_EXPORT_NAME, which we caught here). So we need both assertions at the end of the series, and I'm comfortable with introducing them in separate patches. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
