[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 08/14] block/nbd: Accept SocketAddress
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v3 08/14] block/nbd: Accept SocketAddress |
Date: |
Wed, 15 Jun 2016 16:40:20 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
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 <address@hidden>
>> ---
>> 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
signature.asc
Description: OpenPGP digital signature