Am 28.10.2016 um 14:47 hat Ashijeet Acharya geschrieben:
> Make NFS block driver use various fine grained runtime_opts.
> Set .bdrv_parse_filename() to nfs_parse_filename() and introduce two
> new functions nfs_parse_filename() and nfs_parse_uri() to help parsing
> the URI.
> Add a new option "server" which then accepts a new struct NFSServer.
> "host" is supported as a legacy option and is mapped to its NFSServer
> representation.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacha...@gmail.com>
This patch doesn't apply any more and must be rebased. And even when you
make it technically apply, it still doesn't compile:
block/nfs.c: In function 'nfs_config':
block/nfs.c:477:5: error: passing argument 2 of 'qdict_crumple' makes pointer
from integer without a cast [-Werror]
crumpled_addr = qdict_crumple(addr, true, errp);
This is because qdict_crumple() as it ended up in master has only two
arguments.
> @@ -228,15 +354,49 @@ static int coroutine_fn nfs_co_flush(BlockDriverState
> *bs)
> return task.ret;
> }
>
> -/* TODO Convert to fine grained options */
> static QemuOptsList runtime_opts = {
> .name = "nfs",
> .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> .desc = {
> {
> - .name = "filename",
> + .name = "host",
> + .type = QEMU_OPT_STRING,
> + .help = "Host to connect to",
> + },
This "legacy option" as the commit message calls it, is introduced only
now. Maybe we should completely leave it out. It could make some sense
as a convenience feature, but we can still add that later, and the
existing convenience syntax is the URI, so maybe we don't need a second
one.
> + {
> + .name = "path",
> .type = QEMU_OPT_STRING,
> - .help = "URL to the NFS file",
> + .help = "Path of the image on the host",
> + },
> + {
> + .name = "uid",
> + .type = QEMU_OPT_NUMBER,
> + .help = "UID value to use when talking to the server",
> + },
> + {
> + .name = "gid",
> + .type = QEMU_OPT_NUMBER,
> + .help = "GID value to use when talking to the server",
> + },
> + {
> + .name = "tcp-syncnt",
> + .type = QEMU_OPT_NUMBER,
> + .help = "Number of SYNs to send during the session establish",
> + },
> + {
> + .name = "readahead",
> + .type = QEMU_OPT_NUMBER,
> + .help = "Set the readahead size in bytes",
> + },
> + {
> + .name = "pagecache",
> + .type = QEMU_OPT_NUMBER,
> + .help = "Set the pagecache size in bytes",
> + },
> + {
> + .name = "debug",
> + .type = QEMU_OPT_NUMBER,
> + .help = "Set the NFS debug level (max 2)",
> },
> { /* end of list */ }
> },
> @@ -279,25 +439,94 @@ static void nfs_file_close(BlockDriverState *bs)
> nfs_client_close(client);
> }
>
> -static int64_t nfs_client_open(NFSClient *client, const char *filename,
> +static bool nfs_process_legacy_socket_options(QDict *output_opts,
> + QemuOpts *legacy_opts,
> + Error **errp)
> +{
> + const char *host = qemu_opt_get(legacy_opts, "host");
> + const char *inet = qemu_opt_get(legacy_opts, "inet");
"inet" is not an option in runtime_opts, so this is always NULL.
> + if (!host && inet) {
> + error_setg(errp, "No hostname was specified");
> + return false;
> + }
> + if (host && !inet) {
> + error_setg(errp, "No transportation type was specified");
> + return false;
> + }
> +
> + if (host) {
> + qdict_put(output_opts, "server.host", qstring_from_str(host));
> + qdict_put(output_opts, "server.type", qstring_from_str(inet));
If you want this to be a convenience feature, you might want to set it
to qstring_from_str("inet"), i.e. use the literal string "inet" like in
nfs_parse_uri().
But probably the best is to leave out this function completely and to
support only explicit "server.type" and "server.host".
> + }
> +
> + return true;
> +}
> +
> +static NFSServer *nfs_config(QDict *options, Error **errp)
> +{
> + NFSServer *inet = NULL;
> + QDict *addr = NULL;
> + QObject *crumpled_addr = NULL;
> + Visitor *iv = NULL;
> + Error *local_error = NULL;
> +
> + qdict_extract_subqdict(options, &addr, "server.");
> + if (!qdict_size(addr)) {
> + error_setg(errp, "NFS server address missing");
> + goto out;
> + }
> +
> + crumpled_addr = qdict_crumple(addr, true, errp);
> + if (!crumpled_addr) {
> + goto out;
> + }
> +
> + iv = qobject_input_visitor_new(crumpled_addr, true);
> + visit_type_NFSServer(iv, NULL, &inet, &local_error);
> + if (local_error) {
> + error_propagate(errp, local_error);
> + goto out;
> + }
> +
> +out:
> + QDECREF(addr);
> + qobject_decref(crumpled_addr);
> + visit_free(iv);
> + return inet;
> +}
> +
> +
> +static int64_t nfs_client_open(NFSClient *client, QDict *options,
> int flags, Error **errp, int open_flags)
> {
> - int ret = -EINVAL, i;
> + int ret = -EINVAL;
> + QemuOpts *opts = NULL;
> + Error *local_err = NULL;
> struct stat st;
> - URI *uri;
> - QueryParams *qp = NULL;
> char *file = NULL, *strp = NULL;
>
> - uri = uri_parse(filename);
> - if (!uri) {
> - error_setg(errp, "Invalid URL specified");
> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> + qemu_opts_absorb_qdict(opts, options, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + ret = -EINVAL;
> goto fail;
> }
> - if (!uri->server) {
> - error_setg(errp, "Invalid URL specified");
> +
> + if (!nfs_process_legacy_socket_options(options, opts, errp)) {
> + ret = -EINVAL;
> goto fail;
> }
> - strp = strrchr(uri->path, '/');
> +
> + client->path = g_strdup(qemu_opt_get(opts, "path"));
> + if (!client->path) {
> + ret = -EINVAL;
> + error_setg(errp, "No path was specified");
> + goto fail;
> + }
> +
> + strp = strrchr(client->path, '/');
> if (strp == NULL) {
> error_setg(errp, "Invalid URL specified");
> goto fail;
> @@ -305,85 +534,89 @@ static int64_t nfs_client_open(NFSClient *client, const
> char *filename,
> file = g_strdup(strp);
> *strp = 0;
>
> + /* Pop the config into our state object, Exit if invalid */
> + client->inet = nfs_config(options, errp);
This isn't an InetSocketAddress, so maybe client->server rather than
client->inet.
> + if (!client->inet) {
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> client->context = nfs_init_context();
> if (client->context == NULL) {
> error_setg(errp, "Failed to init NFS context");
> goto fail;
> }
>
> - qp = query_params_parse(uri->query);
> - for (i = 0; i < qp->n; i++) {
> - unsigned long long val;
> - if (!qp->p[i].value) {
> - error_setg(errp, "Value for NFS parameter expected: %s",
> - qp->p[i].name);
> + if (qemu_opt_get(opts, "uid")) {
> + client->uid = qemu_opt_get_number(opts, "uid", 0);
> + nfs_set_uid(client->context, client->uid);
> + }
> +
> + if (qemu_opt_get(opts, "gid")) {
> + client->gid = qemu_opt_get_number(opts, "gid", 0);
> + nfs_set_gid(client->context, client->gid);
> + }
> +
> + if (qemu_opt_get(opts, "tcp-syncnt")) {
> + client->tcp_syncnt = qemu_opt_get_number(opts, "tcp-syncnt", 0);
> + nfs_set_tcp_syncnt(client->context, client->tcp_syncnt);
> + }
> +
> +#ifdef LIBNFS_FEATURE_READAHEAD
> + if (qemu_opt_get(opts, "readahead")) {
> + if (open_flags & BDRV_O_NOCACHE) {
> + error_setg(errp, "Cannot enable NFS readahead "
> + "if cache.direct = on");
> goto fail;
> }
> - if (parse_uint_full(qp->p[i].value, &val, 0)) {
> - error_setg(errp, "Illegal value for NFS parameter: %s",
> - qp->p[i].name);
> - goto fail;
> + client->readahead = qemu_opt_get_number(opts, "readahead", 0);
> + if (client->readahead > QEMU_NFS_MAX_READAHEAD_SIZE) {
> + error_report("NFS Warning: Truncating NFS readahead "
> + "size to %d", QEMU_NFS_MAX_READAHEAD_SIZE);
> + client->readahead = QEMU_NFS_MAX_READAHEAD_SIZE;
> }
> - if (!strcmp(qp->p[i].name, "uid")) {
> - nfs_set_uid(client->context, val);
> - } else if (!strcmp(qp->p[i].name, "gid")) {
> - nfs_set_gid(client->context, val);
> - } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
> - nfs_set_tcp_syncnt(client->context, val);
> -#ifdef LIBNFS_FEATURE_READAHEAD
> - } else if (!strcmp(qp->p[i].name, "readahead")) {
> - if (open_flags & BDRV_O_NOCACHE) {
> - error_setg(errp, "Cannot enable NFS readahead "
> - "if cache.direct = on");
> - goto fail;
> - }
> - if (val > QEMU_NFS_MAX_READAHEAD_SIZE) {
> - error_report("NFS Warning: Truncating NFS readahead"
> - " size to %d", QEMU_NFS_MAX_READAHEAD_SIZE);
> - val = QEMU_NFS_MAX_READAHEAD_SIZE;
> - }
> - nfs_set_readahead(client->context, val);
> + nfs_set_readahead(client->context, client->readahead);
> #ifdef LIBNFS_FEATURE_PAGECACHE
> - nfs_set_pagecache_ttl(client->context, 0);
> + nfs_set_pagecache_ttl(client->context, 0);
> #endif
> - client->cache_used = true;
> + client->cache_used = true;
> + }
> #endif
> +
> #ifdef LIBNFS_FEATURE_PAGECACHE
> - nfs_set_pagecache_ttl(client->context, 0);
> - } else if (!strcmp(qp->p[i].name, "pagecache")) {
> - if (open_flags & BDRV_O_NOCACHE) {
> - error_setg(errp, "Cannot enable NFS pagecache "
> - "if cache.direct = on");
> - goto fail;
> - }
> - if (val > QEMU_NFS_MAX_PAGECACHE_SIZE) {
> - error_report("NFS Warning: Truncating NFS pagecache"
> - " size to %d pages",
> QEMU_NFS_MAX_PAGECACHE_SIZE);
> - val = QEMU_NFS_MAX_PAGECACHE_SIZE;
> - }
> - nfs_set_pagecache(client->context, val);
> - nfs_set_pagecache_ttl(client->context, 0);
> - client->cache_used = true;
> + if (qemu_opt_get(opts, "pagecache")) {
> + if (open_flags & BDRV_O_NOCACHE) {
> + error_setg(errp, "Cannot enable NFS pagecache "
> + "if cache.direct = on");
> + goto fail;
> + }
> + client->pagecache = qemu_opt_get_number(opts, "pagecache", 0);
> + if (client->pagecache > QEMU_NFS_MAX_PAGECACHE_SIZE) {
> + error_report("NFS Warning: Truncating NFS pagecache "
> + "size to %d pages", QEMU_NFS_MAX_PAGECACHE_SIZE);
> + client->pagecache = QEMU_NFS_MAX_PAGECACHE_SIZE;
> + }
> + nfs_set_pagecache(client->context, client->pagecache);
> + nfs_set_pagecache_ttl(client->context, 0);
> + client->cache_used = true;
> + }
> #endif
> +
> #ifdef LIBNFS_FEATURE_DEBUG
> - } else if (!strcmp(qp->p[i].name, "debug")) {
> - /* limit the maximum debug level to avoid potential flooding
> - * of our log files. */
> - if (val > QEMU_NFS_MAX_DEBUG_LEVEL) {
> - error_report("NFS Warning: Limiting NFS debug level"
> - " to %d", QEMU_NFS_MAX_DEBUG_LEVEL);
> - val = QEMU_NFS_MAX_DEBUG_LEVEL;
> - }
> - nfs_set_debug(client->context, val);
> -#endif
> - } else {
> - error_setg(errp, "Unknown NFS parameter name: %s",
> - qp->p[i].name);
> - goto fail;
> + if (qemu_opt_get(opts, "debug")) {
> + client->debug = qemu_opt_get_number(opts, "debug", 0);
> + /* limit the maximum debug level to avoid potential flooding
> + * of our log files. */
> + if (client->debug > QEMU_NFS_MAX_DEBUG_LEVEL) {
> + error_report("NFS Warning: Limiting NFS debug level "
> + "to %d", QEMU_NFS_MAX_DEBUG_LEVEL);
> + client->debug = QEMU_NFS_MAX_DEBUG_LEVEL;
> }
> + nfs_set_debug(client->context, client->debug);
> }
> +#endif
>
> - ret = nfs_mount(client->context, uri->server, uri->path);
> + ret = nfs_mount(client->context, client->inet->host, client->path);
> if (ret < 0) {
> error_setg(errp, "Failed to mount nfs share: %s",
> nfs_get_error(client->context));
> @@ -417,13 +650,11 @@ static int64_t nfs_client_open(NFSClient *client, const
> char *filename,
> client->st_blocks = st.st_blocks;
> client->has_zero_init = S_ISREG(st.st_mode);
> goto out;
> +
> fail:
> nfs_client_close(client);
> out:
> - if (qp) {
> - query_params_free(qp);
> - }
> - uri_free(uri);
> + qemu_opts_del(opts);
> g_free(file);
> return ret;
> }
> @@ -432,28 +663,17 @@ static int nfs_file_open(BlockDriverState *bs, QDict
> *options, int flags,
> Error **errp) {
> NFSClient *client = bs->opaque;
> int64_t ret;
> - QemuOpts *opts;
> - Error *local_err = NULL;
>
> client->aio_context = bdrv_get_aio_context(bs);
>
> - opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> - qemu_opts_absorb_qdict(opts, options, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - ret = -EINVAL;
> - goto out;
> - }
> - ret = nfs_client_open(client, qemu_opt_get(opts, "filename"),
> + ret = nfs_client_open(client, options,
> (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
> errp, bs->open_flags);
> if (ret < 0) {
> - goto out;
> + return ret;
> }
> bs->total_sectors = ret;
> ret = 0;
> -out:
> - qemu_opts_del(opts);
> return ret;
> }
>
> @@ -475,6 +695,7 @@ static int nfs_file_create(const char *url, QemuOpts
> *opts, Error **errp)
> int ret = 0;
> int64_t total_size = 0;
> NFSClient *client = g_new0(NFSClient, 1);
> + QDict *options = NULL;
>
> client->aio_context = qemu_get_aio_context();
>
> @@ -482,7 +703,13 @@ static int nfs_file_create(const char *url, QemuOpts
> *opts, Error **errp)
> total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> BDRV_SECTOR_SIZE);
>
> - ret = nfs_client_open(client, url, O_CREAT, errp, 0);
> + options = qdict_new();
> + ret = nfs_parse_uri(url, options, errp);
> + if (ret < 0) {
> + goto out;
> + }
> +
> + ret = nfs_client_open(client, options, O_CREAT, errp, 0);
> if (ret < 0) {
> goto out;
> }
> @@ -564,6 +791,49 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
> return 0;
> }
>
> +static void nfs_refresh_filename(BlockDriverState *bs, QDict *options)
> +{
> + NFSClient *client = bs->opaque;
> + QDict *opts = qdict_new();
> + QObject *inet_qdict;
> + Visitor *ov;
> +
> + qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("nfs")));
qdict_put(opts, "driver", qstring_from_str("nfs"));
Same for the other occurrences.
> + ov = qobject_output_visitor_new(&inet_qdict);
> + visit_type_NFSServer(ov, NULL, &client->inet, &error_abort);
> + visit_complete(ov, &client->inet);
> + assert(qobject_type(inet_qdict) == QTYPE_QDICT);
> +
> + qdict_put_obj(opts, "server", inet_qdict);
> + qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(client->path)));
> +
> + if (client->uid) {
> + qdict_put_obj(opts, "uid", QOBJECT(qint_from_int(client->uid)));
> + }
> + if (client->gid) {
> + qdict_put_obj(opts, "gid", QOBJECT(qint_from_int(client->gid)));
> + }
> + if (client->tcp_syncnt) {
> + qdict_put_obj(opts, "tcp-syncnt",
> + QOBJECT(qint_from_int(client->tcp_syncnt)));
> + }
> + if (client->readahead) {
> + qdict_put_obj(opts, "readahead",
> + QOBJECT(qint_from_int(client->readahead)));
> + }
> + if (client->pagecache) {
> + qdict_put_obj(opts, "pagecache",
> + QOBJECT(qint_from_int(client->pagecache)));
> + }
> + if (client->debug) {
> + qdict_put_obj(opts, "debug", QOBJECT(qint_from_int(client->debug)));
> + }
> +
> + qdict_flatten(opts);
> + bs->full_open_options = opts;
> +}
This function needs to set bs->exact_filename if the options can be
represented in a URI (like in NBD).
And actually, qemu-iotests 104 fails earlier so that the effect of the
missing bs->exact_filename can't be seen:
+qemu-img: qapi/qobject-output-visitor.c:206: qobject_output_complete:
Assertion `opaque == qov->result' failed.
+./common.config: line 119: 2093 Aborted (core dumped) ( exec
"$QEMU_IMG_PROG" $QEMU_IMG_OPTIONS "$@" )
Kevin
