[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 2/2] qapi: Allow blockdev-add for NBD
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH 2/2] qapi: Allow blockdev-add for NBD |
Date: |
Wed, 3 Feb 2016 18:00:49 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 |
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 <address@hidden>
>> ---
>> 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
[Qemu-block] [PATCH 1/2] block/nbd: Reject port parameter without host, Max Reitz, 2016/02/03