Re: [Qemu-block] [PATCH v5 7/9] nbd: Implement NBD_OPT_GO on client

[Eric Blake]

On 07/19/2017 11:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> 07.07.2017 23:30, Eric Blake wrote:
> 
> [..]
> 
>>
>> +/* Returns -1 if NBD_OPT_GO proves the export @wantname cannot be
>> + * used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
>> + * NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
>> + * go (with @info populated). */
>> +static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
>> +                      NBDExportInfo *info, Error **errp)
>> +{

>> +        if (reply.type == NBD_REP_ACK) {
>> +            /* Server is done sending info and moved into transmission
>> +               phase, but make sure it sent flags */
>> +            if (len) {
>> +                error_setg(errp, "server sent invalid NBD_REP_ACK");
>> +                nbd_send_opt_abort(ioc);
> 
> server is already in transmission phase, so we cant send any options
> more, shouldn't CMD_DISC be send here instead?
> 
>> +                return -1;
>> +            }
>> +            if (!info->flags) {
>> +                error_setg(errp, "broken server omitted
>> NBD_INFO_EXPORT");
>> +                nbd_send_opt_abort(ioc);
> 
> and here

For NBD_OPT_INFO, receipt of NBD_REP_ACK implies the server is still in
negotiation phase; transmission phases is only technically possible on
NBD_OPT_GO.  That said, either error condition means the server is
buggy, so it really doesn't matter what we do in response (we don't know
if the server moved on to transmission phase or not, because it has
already broken protocol by sending us garbage) - so trying to be
courteous to tell the server we don't like their garbage vs. just
silently disconnecting makes no real difference; even if the server
doesn't like what we send (because it thinks we are out of phase), the
server already broke protocol first.  Either way, we're going to
disconnect, and the server has to mop up after its own bugs.

I don't think a followup patch is essential, but I'd also be okay with a
patch that just eliminates the NBD_OPT_ABORT attempt and does a silent
disconnect instead.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature