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 <mre...@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...
signature.asc
Description: OpenPGP digital signature