Eric Blake <ebl...@redhat.com> writes:
> On 03/29/2017 11:45 AM, 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" } } }
>
> This part makes sense, and means this has to go in 2.9.
>
>>
>> Since the internal interfaces still take SocketAddress, this requires
>> conversion function socket_address_crumple(). It'll go away when I
>> update the interfaces.
>
> This is probably the fastest way forward for 2.9, even if we rip it out
> later in 2.10.
>
>>
>> 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
>
> Here, I'm 50/50 on whether the compatibility gunk is worth it, or
> whether to make a clean break of old configurations, as I'm not sure who
> is using the old configurations. But given the lateness of the change
> (and my recent case of being burned on qom-type during freeze when I was
> not conservative), I agree with your conservative approach of remaining
> compatible, even if it means the patch is larger than desired, and even
> if we rip it out again in 2.10.
Ripping out becomes harder the long we wait.
Since both Paolo and Max prefer to go without the compatibility gunk,
I'll drop it.
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> block/nbd.c | 131
>> ++++++++++++++++++++++++++++++++++++++++-----------
>> qapi/block-core.json | 2 +-
>> 2 files changed, 104 insertions(+), 29 deletions(-)
>>
>
>> @@ -223,17 +224,45 @@ static bool nbd_process_legacy_socket_options(QDict
>> *output_options,
>> const char *path = qemu_opt_get(legacy_opts, "path");
>> const char *host = qemu_opt_get(legacy_opts, "host");
>> const char *port = qemu_opt_get(legacy_opts, "port");
>> + const char *sd_path = qemu_opt_get(legacy_opts, "server.data.path");
>> + const char *sd_host = qemu_opt_get(legacy_opts, "server.data.host");
>> + const char *sd_port = qemu_opt_get(legacy_opts, "server.data.port");
>> + bool no_server = path || host || port;
>> + bool server_data = sd_path || sd_host || sd_port;
>> const QDictEntry *e;
>>
>> - if (!path && !host && !port) {
>> + if (!no_server && !server_data) {
>
> Feels like a double negative, although it's not really serving that
> role. Maybe a better name would be s/no_server/bare/, so that 'bare' is
> now your counterpart to 'server_data'.
If we change our mind and keep the gunk, I'll rename to @bare.
>> + return true;
>> + }
>> +
>> + if (no_server && server_data) {
>> + error_setg(errp, "Cannot use %s and %s at the same time",
>> + "path/host/port", "server.data.*");
>> + return false;
>
> Again, 'bare' makes this conditional read a bit better.
>
>> + }
>> +
>> + if (server_data) {
>> + if (sd_host) {
>> + qdict_put(output_options, "server.host",
>> + qstring_from_str(sd_host));
>> + }
>> + if (sd_port) {
>> + qdict_put(output_options, "server.port",
>> + qstring_from_str(sd_port));
>> + }
>> + if (sd_path) {
>> + qdict_put(output_options, "server.path",
>> + qstring_from_str(sd_path));
>> + }
>
> Wow, I need to rebase my qdict_put_str() stuff again - that ought to be
> something we include early in 2.10, as it touches a lot of stuff.
> Doesn't affect your patch for now, though.
>
> Renaming the local variable is trivial, so whether or not you
> incorporate my naming suggestion,
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Need to drop the R-by along with the compatibility gunk, but thanks
anyway!
