Re: [Qemu-devel] [PATCH v4 06/12] block/nbd: Accept SocketAddress

[Max Reitz <mreitz@redhat.com>]

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

On 13.10.2016 13:42, Kevin Wolf wrote:
> Am 28.09.2016 um 22:55 hat Max Reitz geschrieben:
>> 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.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Not opposed in principle to your change, but we should try to keep the
> naming consistent between NBD and the other block drivers, notably the
> SSH work that is currently going on.
> 
> This patch uses 'address' for the SockAddress, the proposed SSH patch
> uses 'server'. I don't mind too much which one we choose, though I like
> 'server' a bit better. Anyway, we should choose one and stick to it in
> all drivers.

OK, I'll use "server" then. I don't mind either way, so even a weak
opinion is enough to persuade me. ;-)

>>  block/nbd.c                   | 166 ++++++++++++++++++++++++++----------------
>>  tests/qemu-iotests/051.out    |   4 +-
>>  tests/qemu-iotests/051.pc.out |   4 +-
>>  3 files changed, 106 insertions(+), 68 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index cdab20f..449f94e 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -32,6 +32,9 @@
>>  #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-output-visitor.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "qapi/qmp/qjson.h"
>>  #include "qapi/qmp/qint.h"
>> @@ -44,7 +47,8 @@ typedef struct BDRVNBDState {
>>      NbdClientSession client;
>>  
>>      /* For nbd_refresh_filename() */
>> -    char *path, *host, *port, *export, *tlscredsid;
>> +    SocketAddress *saddr;
>> +    char *export, *tlscredsid;
>>  } BDRVNBDState;
>>  
>>  static int nbd_parse_uri(const char *filename, QDict *options)
>> @@ -131,7 +135,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))
> 
> strstart()?

Yep, will use.

>>          {
>>              error_setg(errp, "Option '%s' cannot be used with a file name",
>>                         e->key);
>> @@ -205,50 +211,67 @@ out:
>>      g_free(file);
>>  }
>>  
>> -static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
>> +static bool nbd_process_legacy_socket_options(QDict *output_options,
>> +                                              QemuOpts *legacy_opts,
>> +                                              Error **errp)
>>  {
>> -    SocketAddress *saddr;
>> -
>> -    s->path = g_strdup(qemu_opt_get(opts, "path"));
>> -    s->host = g_strdup(qemu_opt_get(opts, "host"));
>> -    s->port = g_strdup(qemu_opt_get(opts, "port"));
>> -
>> -    if (!s->path == !s->host) {
>> -        if (s->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");
>> +    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");
>> +
>> +    if (path && host) {
>> +        error_setg(errp, "path and host may not be used at the same time");
>> +        return false;
>> +    } else if (path) {
>> +        if (port) {
>> +            error_setg(errp, "port may not be used without host");
>> +            return false;
>>          }
>> -        return NULL;
>> +
>> +        qdict_put(output_options, "address.type", qstring_from_str("unix"));
>> +        qdict_put(output_options, "address.data.path", qstring_from_str(path));
>> +    } else if (host) {
>> +        qdict_put(output_options, "address.type", qstring_from_str("inet"));
>> +        qdict_put(output_options, "address.data.host", qstring_from_str(host));
>> +        qdict_put(output_options, "address.data.port",
>> +                  qstring_from_str(port ?: stringify(NBD_DEFAULT_PORT)));
>>      }
>> -    if (s->port && !s->host) {
>> -        error_setg(errp, "port may not be used without host");
>> -        return NULL;
>> +
>> +    return true;
>> +}
> 
> If both the legacy option and the new one are given, the legacy one
> takes precedence. Intentional?

No. I think it'll be easiest to just return an error when a user is
trying to use both.

Thanks,

Max


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