Re: [Qemu-devel] [PATCH v3 08/14] block/nbd: Accept SocketAddress

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6821 bytes --]

On 15.06.2016 01:14, Eric Blake wrote:
> On 04/06/2016 12:28 PM, Max Reitz wrote:
>> Add a new option "address" to the NBD block driver which accepts a
>> SocketAddress.
>>
>> "path", "host" and "port" are still supported as legacy options and are
>> mapped to their corresponding SocketAddress representation.
> 
> The back-compat work is pretty slick.
> 
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/nbd.c                   | 97 ++++++++++++++++++++++++++-----------------
>>  tests/qemu-iotests/051.out    |  4 +-
>>  tests/qemu-iotests/051.pc.out |  4 +-
>>  3 files changed, 64 insertions(+), 41 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 3adf302..9ae66ba 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -32,6 +32,8 @@
>>  #include "qemu/uri.h"
>>  #include "block/block_int.h"
>>  #include "qemu/module.h"
>> +#include "qapi-visit.h"
>> +#include "qapi/qmp-input-visitor.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "qapi/qmp/qjson.h"
>>  #include "qapi/qmp/qint.h"
>> @@ -128,7 +130,9 @@ static bool nbd_has_filename_options_conflict(QDict *options, Error **errp)
>>          if (!strcmp(e->key, "host")
>>              || !strcmp(e->key, "port")
>>              || !strcmp(e->key, "path")
>> -            || !strcmp(e->key, "export"))
>> +            || !strcmp(e->key, "export")
>> +            || !strcmp(e->key, "address")
>> +            || !strncmp(e->key, "address.", 8))
> 
> May need a tweak if you break before '||'

Will do. (Same for patch 6.)

> 
>>          {
>>              error_setg(errp, "Option '%s' cannot be used with a file name",
>>                         e->key);
>> @@ -202,46 +206,66 @@ out:
>>      g_free(file);
>>  }
>>  
>> +static bool nbd_process_legacy_socket_options(QDict *options, Error **errp)
>> +{
>> +    if (qdict_haskey(options, "path") && qdict_haskey(options, "host")) {
>> +        error_setg(errp, "path and host may not be used at the same time");
>> +        return false;
>> +    } else if (qdict_haskey(options, "path")) {
>> +        if (qdict_haskey(options, "port")) {
>> +            error_setg(errp, "port may not be used without host");
>> +            return false;
>> +        }
>> +
>> +        qdict_put(options, "address.type", qstring_from_str("unix"));
>> +        qdict_change_key(options, "path", "address.data.path");
>> +    } else if (qdict_haskey(options, "host")) {
>> +        qdict_put(options, "address.type", qstring_from_str("inet"));
>> +        qdict_change_key(options, "host", "address.data.host");
>> +        if (!qdict_change_key(options, "port", "address.data.port")) {
>> +            qdict_put(options, "address.data.port",
>> +                      qstring_from_str(stringify(NBD_DEFAULT_PORT)));
>> +        }
> 
> The rewrite from old to modern is rather nice.  I wonder if we could
> then use a generated qapi_visit_SocketAddress instead of manually
> handling all the complication of SocketAddress proper.
> 
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>  static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
>>                                   Error **errp)
>>  {
>> -    SocketAddress *saddr;
>> +    SocketAddress *saddr = NULL;
>> +    QDict *addr = NULL;
>> +    QObject *crumpled_addr;
>> +    QmpInputVisitor *iv;
>> +    Error *local_err = NULL;
>>  
>> -    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
>> -        if (qdict_haskey(options, "path")) {
>> -            error_setg(errp, "path and host may not be used at the same time");
>> -        } else {
>> -            error_setg(errp, "one of path and host must be specified");
>> -        }
>> -        return NULL;
>> +    if (!nbd_process_legacy_socket_options(options, errp)) {
>> +        goto fail;
>>      }
>> -    if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
>> -        error_setg(errp, "port may not be used without host");
>> -        return NULL;
>> +
>> +    qdict_extract_subqdict(options, &addr, "address.");
>> +    if (!qdict_size(addr)) {
>> +        error_setg(errp, "NBD server address missing");
>> +        goto fail;
>>      }
>>  
>> -    saddr = g_new0(SocketAddress, 1);
>> +    crumpled_addr = qdict_crumple(addr, true, errp);
>> +    if (!crumpled_addr) {
>> +        goto fail;
>> +    }
>>  
> 
> Ah, so you ARE depending on Dan's qdict_crumple().

In v2, I was still using my own version (qdict_unflatten()), written
specifically for this. Since both fulfill the same purpose and I got to
review qdict_crumple() before anyone got to review qdict_unflatten(), I
dropped the qdict_unflatten() patch from v3; also, "crumple" is more
imaginative than "unflatten", so it has to be better. :-)

>> -    if (qdict_haskey(options, "path")) {
>> -        UnixSocketAddress *q_unix;
>> -        saddr->type = SOCKET_ADDRESS_KIND_UNIX;
>> -        q_unix = saddr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
>> -        q_unix->path = g_strdup(qdict_get_str(options, "path"));
>> -        qdict_del(options, "path");
>> -    } else {
>> -        InetSocketAddress *inet;
>> -        saddr->type = SOCKET_ADDRESS_KIND_INET;
>> -        inet = saddr->u.inet.data = g_new0(InetSocketAddress, 1);
>> -        inet->host = g_strdup(qdict_get_str(options, "host"));
>> -        if (!qdict_get_try_str(options, "port")) {
>> -            inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
>> -        } else {
>> -            inet->port = g_strdup(qdict_get_str(options, "port"));
>> -        }
>> -        qdict_del(options, "host");
>> -        qdict_del(options, "port");
>> +    iv = qmp_input_visitor_new(crumpled_addr);
>> +    visit_type_SocketAddress(qmp_input_get_visitor(iv), NULL, &saddr,
>> +                             &local_err);
> 
> and you DO use the generated QAPI code.  Cool!
> 
>> +    qmp_input_visitor_cleanup(iv);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        goto fail;
>>      }
>>  
>> +    /* TODO: Detect superfluous (unused) options under in addr */
> 
> Is this TODO addressed anywhere, or mentioned in the commit message?

Not yet (also, it should be either of "under" and "in" in the comment,
but not both).

> In fact, I think it's quite easy to address: qmp_input_visitor_new()
> recently changed signatures (commit fc471c18), so all you have to do is
> pass strict=true to that function as part of rebasing your work, at
> which point you will automatically reject any leftover keys in addr that
> didn't get parsed by SocketAddress.

Sounds good, will look into it!

> Needs a rebase, but looks promising.

Will do, thanks for reviewing!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]