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 <[email protected]>
>> ---
>> 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...
signature.asc
Description: OpenPGP digital signature
