Re: [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4359 bytes --]

On 03.02.2016 17:48, Eric Blake wrote:
> On 02/03/2016 09:33 AM, Max Reitz wrote:
>> We have to introduce a new object (BlockdevOptionsNbd) for several
>> reasons:
>> - Neither of InetSocketAddress nor UnixSocketAddress alone is
>>   sufficient, because both are supported
>> - We cannot use SocketAddress because NBD does not support an fd,
>>   and because it is not a flat union which BlockdevOptionsNbd is
> 
> Can we do it anyways, and just error out/document that fd is unsupported?

Would be possible, if InetSocketAddress's port was optional and if it
was a flat union.

(Note that the port not being optional is not a real issue; it just
means the user cannot omit it when using blockdev-add, but that's not so
bad.)

>> - We cannot use a flat union of InetSocketAddress and
>>   UnixSocketAddress because we would need some kind of discriminator
>>   which we do not have; we could inline the UnixSocketAddress as a
>>   string and then make it an 'alternate' type instead of a union, but
>>   this will not work either, because:
>> - InetSocketAddress itself is not suitable for NBD because the port is
>>   not optional (which it is for NBD) and because it offers more options
>>   (like choosing between ipv4 and ipv6) which NBD does not support.
> 
> That, and qapi doesn't (yet) support the use of a flat union as the
> branch of yet another flat union.
> 
> I'd like to reach the point where we can have a flat union with an
> implicit discriminator (if the discriminator was not present, the
> require a default branch), but don't think it should hold up this patch.
> I also think that future qapi improvements may make it possible to
> retrofit this struct to make the mutual exclusion between host/file more
> obvious during introspection, rather than just by documentation.

The problem here is that we really just want to merge and flatten
{Inet,Unix}SocketAddress into a single union. The discriminator
basically is of which object all the non-optional fields are present.

>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qapi/block-core.json | 28 ++++++++++++++++++++++++++--
>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>
> 
>>  ##
>> +# @BlockdevOptionsNbd
>> +#
>> +# Driver specific block device options for NBD. Either of @host or @path must be
>> +# specified, but not both.
>> +#
>> +# @host:    #optional Connects to the given host using TCP.
>> +#
>> +# @port:    #optional Specifies the TCP port to connect to; may be used only in
>> +#           conjunction with @host. Defaults to 10809.
>> +#
>> +# @path:    #optional Connects to the given Unix socket path.
>> +#
>> +# @export:  #optional Name of the NBD export to open.
> 
> Maybe mention that the default is no export name.

Can do (and will do, because you asked for it), but I thought that "Not
specifying an export name means no export name" to be self-evident. :-)

>> +#
>> +# Since: 2.6
>> +##
>> +{ 'struct': 'BlockdevOptionsNbd',
>> +  'data': { '*host':    'str',
>> +            '*port':    'str',
>> +            '*path':    'str',
>> +            '*export':  'str' } }
> 
> I'm not entirely convinced this is the final representation we want, but
> I can't immediately propose anything nicer.

Ideally, this would just be a SocketAddress. Unfortunately, this is not
possible because SocketAddress is not flat but this has to be.

The representation I guess I'd want under the circumstances would be a
flat union of InetSocketAddress and UnixSocketAddress with a semantic
discriminator as described above (check which non-optional fields are
present).

However, in order for this to work, the code generator would need to
support flat unions with such a semantic discriminator[1], and also the
@port parameter in InetSocketAddress should be optional (which might
require non-trivial changes to existing code that expects an
InetSocketAddress). However, as written above, the port not being
optional would not be too bad.

But in any case, the on-wire interface is stable and defined by
block/nbd.c already, so I believe we can enrich the definition later on.
Maybe we should define @port to be non-optional if @host is specified,
though.

Max


[1] And I don't believe I feel quite comfortable with extending the code
generator...


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]