[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 06/12] block/nbd: Accept SocketAddress
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v4 06/12] block/nbd: Accept SocketAddress |
Date: |
Sat, 15 Oct 2016 19:12:35 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
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 <address@hidden>
>
> 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
signature.asc
Description: OpenPGP digital signature