Re: [Qemu-devel] [RFC v2 for-2.9 08/10] nbd: Tidy up blockdev-add interface

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 03/30/2017 08:15 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.  We
>> already use SocketAddressFlat elsewhere in blockdev=add.  Replace it
>> by SocketAddressFlat while we can (it's new in 2.9) for simplicity and
>> consistency.  For example,
>> 
>>     { "execute": "blockdev-add",
>>       "arguments": { "node-name": "foo", "driver": "nbd",
>>                      "server": { "type": "inet",
>> 		                 "data": { "host": "localhost",
>> 				           "port": "12345" } } } }
>> 
>> becomes
>> 
>>     { "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.
>> 
>
> So far, so good.
>
>> Unfortunately, SocketAddress is also visible in -drive since 2.8.  Add
>> still more gunk to nbd_process_legacy_socket_options().  You can now
>> shorten
>
> Dead commit message comment?  Or did you still leave it in here, with
> one last chance to approve it before ripping it out in v3 for comparison?
>
> /me reads patch - looks like you did not remove the gunk yet on this
> revision
>
>> 
>>     -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
>
> The example is useful, but if you drop the compatibility gunk, you'll
> want to reword this as an intentional change in (heretofore
> undocumented) behavior.
>
> I'm ambivalent on whether to keep or remove the server.* gunk (the
> conservative approach is to keep it for at least 2.9, even if we rip it
> out in 2.10, as we already found out with qom-type that not being
> conservative during hard freeze risks unintentional breakage - but the
> counter-argument is that -drive parameters have been undocumented and
> that libvirt is not using 2.8's server.data, so it is unlikely anyone
> else is either).  Or put another way, we've already taken care of
> back-compat to make sure that the legacy 'host' works, whether or not
> the new form is spelled 'server.host' or 'server.data.host', so breaking
> back-compat with 'server.data.host' should not impact clients that are
> only using the older 'host'.

Yes.

I'd like to use the license to break this undocumented interface offered
Paolo et al, but I feel offering everybody one more opportunity to
object can't hurt.

>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/nbd.c          | 94 +++++++++++++++++++++++++++++++++++++---------------
>>  qapi/block-core.json |  2 +-
>>  2 files changed, 69 insertions(+), 27 deletions(-)
>> 
>
>> @@ -223,12 +223,51 @@ 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 = qdict_get_try_str(output_options,
>> +                                            "server.data.path");
>> +    const char *sd_host = qdict_get_try_str(output_options,
>> +                                            "server.data.host");
>> +    const char *sd_port = qdict_get_try_str(output_options,
>> +                                            "server.data.port");
>> +    bool bare = path || host || port;
>> +    bool server_data = sd_path || sd_host || sd_port;
>
> Ah, so diff from v1 is that you took my suggestion of 'bare' for making
> the compatibility gunk checks easier to read.
>
>
>> +    if (bare && server_data) {
>> +        error_setg(errp, "Cannot use 'server' and path/host/port at the "
>> +                   "same time");
>> +        return false;
>> +    }
>> +
>> +    if (server_data) {
>> +        if (sd_host) {
>> +            val = qdict_get(output_options, "server.data.host");
>> +            qobject_incref(val);
>> +            qdict_put_obj(output_options, "server.host", val);
>> +            qdict_del(output_options, "server.data.host");
>> +        }
>> +        if (sd_port) {
>> +            val = qdict_get(output_options, "server.data.port");
>> +            qobject_incref(val);
>> +            qdict_put_obj(output_options, "server.port", val);
>> +            qdict_del(output_options, "server.data.port");
>> +        }
>> +        if (sd_path) {
>> +            val = qdict_get(output_options, "server.data.path");
>> +            qobject_incref(val);
>> +            qdict_put_obj(output_options, "server.path", val);
>> +            qdict_del(output_options, "server.data.path");
>> +        }
>> +        return true;
>
> This exits early, possibly setting both server.host and server.path, and
> misses out on the fact that 'bare' mode flags:
>
>     if (path && host) {
>         error_setg(errp, "path and host may not be used at the same time");
>         return false;
>
> If I'm understanding it right, this will still be flagged later during
> the QAPI type visit as invalid, although the error message quality may
> be different.

s/different/not so hot/

> Would it be any better to just use the same validation logic for both,
> by instead writing:
>
> if (server_data) {
>     if (sd_host) {
>         host = sd_host;
>     }
>     if (sd_port) {
>         port = sd_port;
>     }
>     if (sd_path) {
>         path = sd_path;
>     }
> } else {
>
>> +    }
>> +
>> +    assert(bare);
>> +
>>      for (e = qdict_first(output_options); e; e = qdict_next(output_options, e))
>>      {
>>          if (strstart(e->key, "server.", NULL)) {
>
> to enclose this for loop to only happen on the bare branch, having both
> branches then fall into the validation code?

My reason for doing it this way is to avoid changing the existing gunk
as much as I possibly can, even when that leads to suboptimal error
messages.

>> @@ -248,20 +287,21 @@ static bool nbd_process_legacy_socket_options(QDict *output_options,
>>          }
>>  
>>          qdict_put(output_options, "server.type", qstring_from_str("unix"));
>> -        qdict_put(output_options, "server.data.path", qstring_from_str(path));
>> +        qdict_put(output_options, "server.path", qstring_from_str(path));
>>      } else if (host) {
>>          qdict_put(output_options, "server.type", qstring_from_str("inet"));
>> -        qdict_put(output_options, "server.data.host", qstring_from_str(host));
>> -        qdict_put(output_options, "server.data.port",
>> +        qdict_put(output_options, "server.host", qstring_from_str(host));
>> +        qdict_put(output_options, "server.port",
>>                    qstring_from_str(port ?: stringify(NBD_DEFAULT_PORT)));
>
> Perhaps another reason to fall through: server.port is mandatory in the
> schema (for the 'inet' branch of the union), but 'port' is optional on
> the command line by populating the default here.  Returning early on the
> server_data form makes 'server.data.port' mandatory, if you use the
> server.* form; I guess it's a question of whether we want the server.*
> form to match the bare form, or whether it can be as strict as the QMP
> form and only the bare form has to have compatibility gunk.  Or maybe,
> as argued by others, we don't want 'server.data'*' back-compat at all.

If we decide to keep the new compatibility gunk (and I hope we won't),
we can improve it in 2.10.