Libguestfs - Aug 2018 - Re: [PATCH nbdkit v2] protocol: Implement NBD_OPT_GO.

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;
> +  }
> +
>    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.

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.)

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).

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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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

On Mon, Aug 06, 2018 at 09:02:14AM -0500, Eric Blake
wrote:
> 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.
Understood.  I initially believed it was reachable if nr_options =MAX_NR_OPTIONS
but we check that case previously.

Thanks for the review,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW