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

[Max Reitz]

On 03.02.2016 18:06, Daniel P. Berrange wrote:
> On Wed, Feb 03, 2016 at 05:33:16PM +0100, 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
> 
> With my patch series that converts NBD to use QIOChannel, all the
> entry points for client & server *do* take a SocketAddress struct
> to provide address info. So internally the code does in fact allow
> use of an FD, if there were a way to specify it a the QAPI level...
> 
> eg see
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04159.html
> 
>> - 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.
> 
> The *should* support ipv4 and ipv6 options for NBD. We should also make
> the port optional in the SocketAddress struct - I tried to do that previously
> but my patch was flawed, but we should revisit this.
> 
> So IMHO all the things you list above are reasons *for* using SocketAddress
> and not re-inventing it poorly with explicit host + port fields.

That's good news, thanks!

However, the issue remains that the NBD block driver expects flattened
options which is syntactically incompatible to SocketAddress. Maybe the
best way to address this would be to just make block/nbd.c directly
accept a SocketAddress and keep the old flattened @host, @port, and
@path options only as a legacy mapping to inet.host, inet.port, and
unix.path.

Anyway, I'll wait for your series to get merged, then.

Max

signature.asc
Description: OpenPGP digital signature