On 04/25/2016 03:47 AM, Alex Bligh wrote: > > On 23 Apr 2016, at 00:40, Eric Blake <ebl...@redhat.com> wrote: > >> NBD commit 6d34500b clarified how clients and servers are supposed >> to behave before closing a connection. It added NBD_REP_ERR_SHUTDOWN >> (for the server to announce it is about to go away during option >> haggling, so the client should quit sending NBD_OPT_* other than >> NBD_OPT_ABORT) and ESHUTDOWN (for the server to announce it is about >> to go away during transmission, so the client should quit sending >> NBD_CMD_* other than NBD_CMD_DISC). It also clarified that >> NBD_OPT_ABORT gets a reply, while NBD_CMD_DISC does not. >> >> This patch merely adds the missing reply to NBD_OPT_ABORT and teaches >> the client to recognize server errors. Actually teaching the server >> to send NBD_REP_ERR_SHUTDOWN or ESHUTDOWN would require knowing that >> the server has been requested to shut down soon (maybe we could do >> that by installing a SIGINT handler in qemu-nbd, which transitions >> from RUNNING to a new state that waits for the client to react, >> rather than just out-right quitting). >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >> ---
>> @@ -484,6 +486,10 @@ static int nbd_negotiate_options(NBDClient *client)
>> if (ret < 0) {
>> return ret;
>> }
>> + /* Let the client keep trying, unless they asked to quit */
>> + if (clientflags == NBD_OPT_ABORT) {
>
> OK that's totally confusing. clientflags is not the client flags. clientflags
> is the NBD option ID, which happens to be the two bytes after the NBD OPT
> magic,
> which is the client flags if we were doing oldstyle negotiation, not newstyle
> negotiation.
Yes, 'clientflags' is a poor name; I should rename it in a separate
patch. It is the option-negotiation command type.
>
> Except:
>
>> + return -EINVAL;
>> + }
>> break;
>> }
>> } else if (fixedNewstyle) {
>
> So the above is for NewStyle (not fixedNewStyle)?
The above is for fixedNewStyle when TLS is not negotiated yet; the 'else
if' is for fixedNewStyle after TLS has been negotiated. Prior to TLS,
we have to special-case NBD_OPT_ABORT and actually quit.
>
> In which case more than one option isn't even supported, so what's the
> stuff purporting to handle TLS doing there?
>
> Confused ...
Sounds like a cleanup patch as a prerequisite on my next respin would
help, then.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
