Max Reitz <mre...@redhat.com> writes:
> On 30.03.2017 15:15, Markus Armbruster wrote:
>> Commit 831acdc "sheepdog: Implement bdrv_parse_filename()" and commit
>> d282f34 "sheepdog: Support blockdev-add" have different ideas on how
>> the QemuOpts parameters for the server address are named. Fix that.
>> While there, rename BlockdevOptionsSheepdog member addr to server, for
>> consistency with BlockdevOptionsSsh, BlockdevOptionsGluster,
>> BlockdevOptionsNbd.
>>
>> Commit 831acdc's example becomes
>>
>> --drive
>> driver=sheepdog,server.type=inet,server.host=fido,server.port=7000,vdi=dolly
>>
>> instead of
>>
>> --drive driver=sheepdog,host=fido,vdi=dolly
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> block/sheepdog.c | 84
>> ++++++++++++++++++++++++++++++++++------------------
>> qapi/block-core.json | 4 +--
>> 2 files changed, 58 insertions(+), 30 deletions(-)
>
> There's trailing whitespace somewhere in this patch (git-am complained).
Fixe in my tree.
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index 89e98ed..c81013d 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -13,9 +13,11 @@
>> */
>>
>> #include "qemu/osdep.h"
>> +#include "qapi-visit.h"
>> #include "qapi/error.h"
>> #include "qapi/qmp/qdict.h"
>> #include "qapi/qmp/qint.h"
>> +#include "qapi/qobject-input-visitor.h"
>> #include "qemu/uri.h"
>> #include "qemu/error-report.h"
>> #include "qemu/sockets.h"
>> @@ -547,6 +549,47 @@ static SocketAddress *sd_socket_address(const char
>> *path,
>> return addr;
>> }
>>
>> +static SocketAddress *sd_server_config(QDict *options, Error **errp)
>> +{
>> + QDict *server = NULL;
>> + QObject *crumpled_server = NULL;
>> + Visitor *iv = NULL;
>> + SocketAddressFlat *saddr_flat = NULL;
>> + SocketAddress *saddr = NULL;
>> + Error *local_err = NULL;
>> +
>> + qdict_extract_subqdict(options, &server, "server.");
>
> server may be empty:
>
> $ ./qemu-img info sheepdog:vdi
> qemu-img: Could not open 'sheepdog:vdi': Parameter 'type' is missing
>
> I'd propose creating an 'inet' SocketAddress object then for
> SD_DEFAULT_ADDR and SD_DEFAULT_PORT so that existing behavior won't change.
Makes sense.
In my working tree:
$ upstream-qemu-img info sheepdog:vdi
@@@ server.host=localhost
@@@ server.port=7000
@@@ tag=
@@@ server.type=inet
@@@ vdi=vdi
### vdi=vdi addr=localhost:7000 snap-id=(null) tag=
upstream-qemu-img: Could not open 'sheepdog:vdi': Failed to connect socket:
Connection refused
>> +
>> + crumpled_server = qdict_crumple(server, errp);
>> + if (!crumpled_server) {
>> + goto done;
>> + }
>> +
>> + /*
>> + * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
>> + * server.type=inet. .to doesn't matter, it's ignored anyway.
>> + * That's because when @options come from -blockdev or
>> + * blockdev_add, members are typed according to the QAPI schema,
>> + * but when they come from -drive, they're all QString. The
>> + * visitor expects the former.
>> + */
>> + iv = qobject_input_visitor_new(crumpled_server);
>> + visit_type_SocketAddressFlat(iv, NULL, &saddr_flat, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + goto done;
>> + }
>> +
>> + saddr = socket_address_crumple(saddr_flat);
>> +
>> +done:
>> + qapi_free_SocketAddressFlat(saddr_flat);
>> + visit_free(iv);
>> + qobject_decref(crumpled_server);
>> + QDECREF(server);
>> + return saddr;
>> +}
>> +
>> /* Return -EIO in case of error, file descriptor on success */
>> static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
>> {
>> @@ -1175,15 +1218,17 @@ static void sd_parse_filename(const char *filename,
>> QDict *options,
>> }
>>
>> if (cfg.host) {
>> - qdict_set_default_str(options, "host", cfg.host);
>> + qdict_set_default_str(options, "server.host", cfg.host);
>> + qdict_set_default_str(options, "server.type", "inet");
>> }
>> if (cfg.port) {
>> snprintf(buf, sizeof(buf), "%d", cfg.port);
>> - qdict_set_default_str(options, "port", buf);
>> + qdict_set_default_str(options, "server.port", buf);
>
> .port is mandatory, so doing this optionally results in:
>
> $ ./qemu-img info sheepdog://localhost/foo
> qemu-img: Could not open 'sheepdog://localhost/foo': Parameter 'port' is
> missing
In my working tree:
$ upstream-qemu-img info sheepdog://localhost6/foo
@@@ server.host=localhost6
@@@ server.port=7000
@@@ tag=
@@@ server.type=inet
@@@ vdi=foo
### vdi=foo addr=localhost6:7000 snap-id=(null) tag=
upstream-qemu-img: Could not open 'sheepdog://localhost6/foo': Failed to
connect socket: Connection refused
$ upstream-qemu-img info sheepdog://:123/foo
@@@ server.host=localhost
@@@ server.port=123
@@@ tag=
@@@ server.type=inet
@@@ vdi=foo
### vdi=foo addr=localhost:123 snap-id=(null) tag=
upstream-qemu-img: Could not open 'sheepdog://:123/foo': Failed to connect
socket: Connection refused
$ upstream-qemu-img info sheepdog:///foo
@@@ server.host=localhost
@@@ server.port=7000
@@@ tag=
@@@ server.type=inet
@@@ vdi=foo
### vdi=foo addr=localhost:7000 snap-id=(null) tag=
upstream-qemu-img: Could not open 'sheepdog:///foo': Failed to connect
socket: Connection refused
>> }
>> if (cfg.path) {
>> - qdict_set_default_str(options, "path", cfg.path);
>> - }
>> + qdict_set_default_str(options, "server.path", cfg.path);
>> + qdict_set_default_str(options, "server.type", "unix");
>> + }
>
> Indentation is off.
Fixing...
> Max
Thanks!
[...]
