[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 36/44] nbd: Improve handling of shutdown requ

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v3 36/44] nbd: Improve handling of shutdown requests
Date: Mon, 25 Apr 2016 13:20:21 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

On 04/25/2016 03:47 AM, Alex Bligh wrote:
> On 23 Apr 2016, at 00:40, Eric Blake <address@hidden> 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 <address@hidden>
>> ---

>> @@ -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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]