[Libguestfs] [PATCH nbdkit v2] protocol: Implement NBD_OPT_GO.

[Eric Blake]

On 08/06/2018 07:03 AM, Richard W.M. Jones wrote:
> Actually I was wrong, there *is* one substantive change over v1, which
> is this:
> 
>> +  /* The client is buggy.  The last option must be NBD_OPT_GO or
>> +   * NBD_OPT_EXPORT_NAME.
>> +   */
>> +  else {
>> +    nbdkit_error ("client options list didn't finish with NBD_OPT_GO "
>> +           "or NBD_OPT_EXPORT_NAME");
>> +    return -1;
>> +  }

As I pointed out in the other mail, this else clause is unreachable.

>> +
>>     return 0;
> 
> Current behaviour in nbdkit is that if the last requested option is a
> recognized option, but is not NBD_OPT_EXPORT_NAME then nbdkit will
> send the handshake_finish struct and continue to transport mode.

In the current code, there is no such thing as a last option except for 
EXPORT_NAME - you are in a for loop that is only broken when you exceed 
too many options (in which case you kill the connection) or when you 
encountered EXPORT_NAME.

> 
> However I don't think nbdkit is currently behaving correctly.
> 
> In any case the proposed behaviour (above) is that in this case we
> will close the connection abruptly and record an error in the log.
> (There's no way to return an error back to the client when we get into
> this situation.)

And how can we get in that situation in the first place?

> 
> I am not certain if this is correct or not, but I believe it is: the
> NBD protocol says the only way to exit option mode and enter
> transmission mode is:
> 
>    "Transmission mode can be entered (by the client sending
>    NBD_OPT_EXPORT_NAME or by the server responding to an NBD_OPT_GO
>    with NBD_REP_ACK)."
> 
> (The other two ways mentioned in that section are about terminating
> the connection completely).
> 

You are correct that if EXPORT_NAME fails, or that if you don't think 
the client is reaching a successful GO fast enough, then your only 
recourse is to hang up on the client (preferably after sending a 
last-ditch error message to the client explaining why, when that is 
still permitted).

> This also implies that the number of options must never be zero, which
> the spec does say in passing:
> 
>    "At this point, we move on to option haggling, during which point
>    the client can send one or (in fixed newstyle) more options to the
>                        ~~~
>    server."
> 
> The current nbdkit code assumes this, so we're OK there.

Indeed - if the client hangs up without having ever sent GO or 
EXPORT_NAME, then they gave up on negotiation first, and we have nothing 
further to say to them.

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