From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60677) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aR0nb-0000qp-Mm for qemu-devel@nongnu.org; Wed, 03 Feb 2016 12:01:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aR0na-0006VW-EW for qemu-devel@nongnu.org; Wed, 03 Feb 2016 12:00:59 -0500 References: <1454517196-4560-1-git-send-email-mreitz@redhat.com> <1454517196-4560-3-git-send-email-mreitz@redhat.com> <56B22F6E.5090308@redhat.com> From: Max Reitz Message-ID: <56B23241.9060301@redhat.com> Date: Wed, 3 Feb 2016 18:00:49 +0100 MIME-Version: 1.0 In-Reply-To: <56B22F6E.5090308@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="uuxKBNDqJubslGjECKC0orjcNw8jN2PFi" Subject: Re: [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-block@nongnu.org Cc: Kevin Wolf , Paolo Bonzini , qemu-devel@nongnu.org, Markus Armbruster This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --uuxKBNDqJubslGjECKC0orjcNw8jN2PFi Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 >=20 > Can we do it anyways, and just error out/document that fd is unsupporte= d? 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 option= s >> (like choosing between ipv4 and ipv6) which NBD does not support. >=20 > That, and qapi doesn't (yet) support the use of a flat union as the > branch of yet another flat union. >=20 > 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= =2E > I also think that future qapi improvements may make it possible to > retrofit this struct to make the mutual exclusion between host/file mor= e > 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 >> --- >> qapi/block-core.json | 28 ++++++++++++++++++++++++++-- >> 1 file changed, 26 insertions(+), 2 deletions(-) >> >=20 >> ## >> +# @BlockdevOptionsNbd >> +# >> +# Driver specific block device options for NBD. Either of @host or @p= ath 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 us= ed 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. >=20 > 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' } } >=20 > I'm not entirely convinced this is the final representation we want, bu= t > 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... --uuxKBNDqJubslGjECKC0orjcNw8jN2PFi Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWsjJBAAoJEDuxQgLoOKytRQgH/RretzlTh3sMfoZDAHdk3ZU6 nxDqwgDmlkRacX/9/7RskrDsFq+3WkfQ6smWeVJhyO7ZlYPvwJwmzhmqor8z5hKm 4Uqxt3ec4OjK0CL9UF3Z0rsy3tZI0vXTTE9nM8H1/SwH62I+GcC/4ClnxvehmpqD bfRlR1tyg1EcjtA+djdH2eu/BrxR5pxTTDnPgCBIHoj9AABXdANy97VA9upC6r/f wwWKDjWHg+uMY1mRUnXHS0KZEnYeh45IBWHxIXl+qP59KxvTw4zEweqZqmiWcK/V DxD3gHiA3u2KBsVZy3HISYRDsNca1sx0eJ4LI7t2gLBJoY/Zlw13zpdeC7528rQ= =1Y4C -----END PGP SIGNATURE----- --uuxKBNDqJubslGjECKC0orjcNw8jN2PFi--