Re: [Qemu-block] [Qemu-devel] [for-2.9 7/8] nbd: Tidy up blockdev-add interface

[Markus Armbruster]

Max Reitz <address@hidden> writes:

> On 29.03.2017 18:45, Markus Armbruster wrote:
>> SocketAddress is a simple union, and simple unions are awkward: they
>> have their variant members wrapped in a "data" object on the wire, and
>> require additional indirections in C.  I intend to limit its use to
>> existing external interfaces, and convert all internal interfaces to
>> SocketAddressFlat.
>> 
>> BlockdevOptionsNbd is an external interface using SocketAddress, but
>> it's new in 2.9.  Replace it by SocketAddressFlat while we can.  This
>> simplifies
>> 
>>     { "execute": "blockdev-add",
>>       "arguments": { "node-name": "foo", "driver": "nbd",
>>                      "server": { "type": "inet",
>>                               "data": { "host": "localhost",
>>                                         "port": "12345" } } } }
>> 
>> to
>> 
>>     { "execute": "blockdev-add",
>>       "arguments": { "node-name": "foo", "driver": "nbd",
>>                      "server": { "type": "inet",
>>                               "host": "localhost", "port": "12345" } } }
>> 
>> Since the internal interfaces still take SocketAddress, this requires
>> conversion function socket_address_crumple().  It'll go away when I
>> update the interfaces.
>> 
>> Unfortunately, SocketAddress is also visible in -drive since 2.8.  Add
>> still more gunk to nbd_process_legacy_socket_options().  You can now
>> shorten
>> 
>>     -drive 
>> if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345
>> 
>> to
>> 
>>     -drive 
>> if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  block/nbd.c          | 131 
>> ++++++++++++++++++++++++++++++++++++++++-----------
>>  qapi/block-core.json |   2 +-
>>  2 files changed, 104 insertions(+), 29 deletions(-)
>
> To be completely frank: If there is any drawback to using
> SocketAddressFlat instead of SocketAdress, I would be against using it.
> Less indirections in C is an argument, but less {} on the wire is not,
> in my opinion. QMP is a machine interface and machines don't care about
> that additional level.
>
> Therefore, if you think this somehow affects compatibility, I would not
> want to do this change.

PRO:

1. Less nesting on the wire

   You think this isn't a valid argument.  I think it is, just a weak
   one.

2. Less indirection in C

   Makes dealing with the data type annoying.  In other words, the C
   interface is poor.  Poor C interfaces hiding in generated code is one
   thing, but SocketAddress is used in C interfaces between handwritten
   code.  Needs to go.

   Once its gone, all uses of SocketAddress we can't change for
   compatibility reasons need to go through a conversion function.  No
   biggie, but still, the fewer we have, the better.

3. Consistency within blockdev-add

   Nowhere else in blockdev-add do we wrap parts of socket addresses in
   "data".  I guess I should mention this in the commit message.

CON:

1. Renames -drive server.data.* to server.*.  These are new in 2.8.
   Libvirt doesn't use them.  I doubt anyone does.  My patch adds
   compatibility gunk anyway.

> I personally agree with Paolo that we can just break what we have, and
> in addition I think that we *should* break what we have or we should not
> do this change at all.

I'm totally fine with ripping out the compatibility gunk.

I put it in v1 so reviewers can like it or shoot it down.  Better than
not having it in v1, and have reviewers demand it for v2 just so they
can decide whether they want it.