From: Kevin Wolf <kwolf@redhat.com>
To: Ashijeet Acharya <ashijeetacharya@gmail.com>
Cc: pl@kamp.de, jcody@redhat.com, mreitz@redhat.com,
armbru@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org,
qemu-block@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 1/2] block/nfs: Introduce runtime_opts in NFS
Date: Mon, 24 Oct 2016 17:10:56 +0200 [thread overview]
Message-ID: <20161024151056.GE4374@noname.redhat.com> (raw)
In-Reply-To: <1476880711-854-2-git-send-email-ashijeetacharya@gmail.com>
Am 19.10.2016 um 14:38 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. This will help us to prepare the NFS for blockdev-add.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
> block/nfs.c | 360 +++++++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 261 insertions(+), 99 deletions(-)
>
> diff --git a/block/nfs.c b/block/nfs.c
> index 8602a44..5eb909e 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -35,8 +35,12 @@
> #include "qemu/uri.h"
> #include "qemu/cutils.h"
> #include "sysemu/sysemu.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qint.h"
> +#include "qapi/qmp/qstring.h"
> #include <nfsc/libnfs.h>
>
> +
> #define QEMU_NFS_MAX_READAHEAD_SIZE 1048576
> #define QEMU_NFS_MAX_PAGECACHE_SIZE (8388608 / NFS_BLKSIZE)
> #define QEMU_NFS_MAX_DEBUG_LEVEL 2
> @@ -61,6 +65,127 @@ typedef struct NFSRPC {
> NFSClient *client;
> } NFSRPC;
>
> +static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
> +{
> + URI *uri = NULL;
> + QueryParams *qp = NULL;
> + int ret = 0, i;
> + const char *p;
> +
> + uri = uri_parse(filename);
> + if (!uri) {
> + error_setg(errp, "Invalid URI specified");
> + ret = -EINVAL;
> + goto out;
> + }
> + if (strcmp(uri->scheme, "nfs") != 0) {
> + error_setg(errp, "URI scheme must be 'nfs'");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!uri->server || strcmp(uri->server, "") == 0) {
No need to use strcmp(), !*uri->server is enough.
> + error_setg(errp, "missing hostname in URI");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!uri->path || strcmp(uri->path, "") == 0) {
> + error_setg(errp, "missing file path in URI");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + p = uri->path ? uri->path : "/";
You just checked that uri->path is non-NULL, so this is dead code.
> + p += strspn(p, "/");
> + if (p[0]) {
> + qdict_put(options, "export", qstring_from_str(p));
> + }
"export" isn't among the recognised options. You may have missed this
code fragment when removing the option from your patch.
> + qp = query_params_parse(uri->query);
> + if (!qp) {
> + error_setg(errp, "could not parse query parameters");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + qdict_put(options, "host", qstring_from_str(uri->server));
> +
> + qdict_put(options, "path", qstring_from_str(uri->path));
> +
> + for (i = 0; i < qp->n; i++) {
> + if (!qp->p[i].value) {
> + error_setg(errp, "Value for NFS parameter expected: %s",
> + qp->p[i].name);
> + goto out;
> + }
> + if (parse_uint_full(qp->p[i].value, NULL, 0)) {
> + error_setg(errp, "Illegal value for NFS parameter: %s",
> + qp->p[i].name);
> + goto out;
> + }
> + if (!strcmp(qp->p[i].name, "uid")) {
> + qdict_put(options, "uid",
> + qstring_from_str(qp->p[i].value));
> + } else if (!strcmp(qp->p[i].name, "gid")) {
> + qdict_put(options, "gid",
> + qstring_from_str(qp->p[i].value));
> + } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
> + qdict_put(options, "tcp-syncnt",
> + qstring_from_str(qp->p[i].value));
> + } else if (!strcmp(qp->p[i].name, "readahead")) {
> + qdict_put(options, "readahead",
> + qstring_from_str(qp->p[i].value));
> + } else if (!strcmp(qp->p[i].name, "pagecache")) {
> + qdict_put(options, "pagecache",
> + qstring_from_str(qp->p[i].value));
> + } else if (!strcmp(qp->p[i].name, "debug")) {
> + qdict_put(options, "debug",
> + qstring_from_str(qp->p[i].value));
> + } else {
> + error_setg(errp, "Unknown NFS parameter name: %s",
> + qp->p[i].name);
> + goto out;
> + }
> + }
> +out:
> + if (qp) {
> + query_params_free(qp);
> + }
> + if (uri) {
> + uri_free(uri);
> + }
> + return ret;
> +}
> +
> +static void nfs_parse_filename(const char *filename, QDict *options,
> + Error **errp)
> +{
> + int ret = 0;
> +
> + if (qdict_haskey(options, "host") ||
> + qdict_haskey(options, "path") ||
> + qdict_haskey(options, "uid") ||
> + qdict_haskey(options, "gid") ||
> + qdict_haskey(options, "tcp-syncnt") ||
> + qdict_haskey(options, "readahead") ||
> + qdict_haskey(options, "pagecache") ||
> + qdict_haskey(options, "debug")) {
> + error_setg(errp, "host/path/uid/gid/tcp-syncnt/readahead/pagecache"
> + "/debug and a filename may not be used at the same"
> + " time");
> + return;
> + }
> +
> + if (strstr(filename, "://")) {
Why this check? It means that any passed filename that doesn't contain
"://" is silently ignored. Shouldn't an error be returned in this case?
(Which would automatically happen if you called nfs_parse_uri()
unconditionally.)
> + ret = nfs_parse_uri(filename, options, errp);
> + if (ret < 0) {
> + error_setg(errp, "No valid URL specified");
> + }
> + return;
> + }
> +}
> +
> static void nfs_process_read(void *arg);
> static void nfs_process_write(void *arg);
>
> @@ -228,15 +353,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",
> + },
> + {
> + .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 +438,40 @@ static void nfs_file_close(BlockDriverState *bs)
> nfs_client_close(client);
> }
>
> -static int64_t nfs_client_open(NFSClient *client, const char *filename,
> +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;
> + const char *host, *path;
> + unsigned long long val;
>
> - uri = uri_parse(filename);
> - if (!uri) {
> - error_setg(errp, "Invalid URL specified");
> - goto fail;
> + 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;
> }
> - if (!uri->server) {
> - error_setg(errp, "Invalid URL specified");
> - goto fail;
> +
> + host = qemu_opt_get(opts, "host");
> + if (!host) {
> + ret = -EINVAL;
> + error_setg(errp, "No hostname was specified");
> + goto out;
> }
> - strp = strrchr(uri->path, '/');
> +
> + path = qemu_opt_get(opts, "path");
> + if (!path) {
> + ret = -EINVAL;
> + error_setg(errp, "No path was specified");
> + goto out;
> + }
> +
> + strp = strrchr(path, '/');
> if (strp == NULL) {
> error_setg(errp, "Invalid URL specified");
> goto fail;
> @@ -305,85 +479,83 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
> file = g_strdup(strp);
> *strp = 0;
>
> - client->context = nfs_init_context();
> - if (client->context == NULL) {
> - error_setg(errp, "Failed to init NFS context");
> - goto fail;
> + if (qemu_opt_get(opts, "uid")) {
> + nfs_set_uid(client->context,
> + qemu_opt_get_number(opts, "uid", getuid()));
> }
The patch puts this nicely together in a single hunk: You can't move the
context initialisation to later, but then use it in nfs_set_uid() here.
Same for the other options that you set before actually initialising the
context.
> - 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, "gid")) {
> + nfs_set_gid(client->context,
> + qemu_opt_get_number(opts, "gid", getgid()));
> + }
> +
> + if (qemu_opt_get(opts, "tcp-syncnt")) {
> + nfs_set_tcp_syncnt(client->context,
> + qemu_opt_get_number(opts, "tcp-syncnt", 0));
> + }
> +
> +#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;
> + val = qemu_opt_get_number(opts, "readahead", 0);
> + 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;
> }
> - 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, val);
> #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;
> + nfs_set_pagecache_ttl(client->context, 0);
> + 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;
> + }
> + val = qemu_opt_get_number(opts, "pagecache", 0);
> + 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;
> + }
> #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")) {
> + val = qemu_opt_get_number(opts, "debug", 0);
> + /* 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
> +
> + client->context = nfs_init_context();
> + if (client->context == NULL) {
> + error_setg(errp, "Failed to init NFS context");
> + goto fail;
> }
>
> - ret = nfs_mount(client->context, uri->server, uri->path);
> + ret = nfs_mount(client->context, host, path);
> if (ret < 0) {
> error_setg(errp, "Failed to mount nfs share: %s",
> nfs_get_error(client->context));
> @@ -417,13 +589,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 +602,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 +634,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;
>
> client->aio_context = qemu_get_aio_context();
>
> @@ -482,7 +642,9 @@ 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 = qemu_opts_to_qdict(opts, NULL);
> +
> + ret = nfs_client_open(client, options, O_CREAT, errp, 0);
> if (ret < 0) {
> goto out;
> }
This doesn't look right. The options that you're passing to
nfs_client_open() now are create options (nfs_create_opts, i.e. only
"size") and don't contain the host name etc. This information is passed
in 'url', which is completely unused now.
> @@ -578,7 +740,7 @@ static BlockDriver bdrv_nfs = {
> .protocol_name = "nfs",
>
> .instance_size = sizeof(NFSClient),
> - .bdrv_needs_filename = true,
> + .bdrv_parse_filename = nfs_parse_filename,
> .create_opts = &nfs_create_opts,
>
> .bdrv_has_zero_init = nfs_has_zero_init,
This was just a quick review, I'll try to do a more thorough one when
the big things are fixed.
Kevin
next prev parent reply other threads:[~2016-10-24 15:11 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-19 12:38 [Qemu-devel] [PATCH 0/2] allow blockdev-add for NFS Ashijeet Acharya
2016-10-19 12:38 ` [Qemu-devel] [PATCH 1/2] block/nfs: Introduce runtime_opts in NFS Ashijeet Acharya
2016-10-24 15:10 ` Kevin Wolf [this message]
2016-10-24 18:42 ` Ashijeet Acharya
2016-10-24 18:52 ` Ashijeet Acharya
2016-10-19 12:38 ` [Qemu-devel] [PATCH 2/2] qapi: allow blockdev-add for NFS Ashijeet Acharya
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161024151056.GE4374@noname.redhat.com \
--to=kwolf@redhat.com \
--cc=armbru@redhat.com \
--cc=ashijeetacharya@gmail.com \
--cc=eblake@redhat.com \
--cc=jcody@redhat.com \
--cc=mreitz@redhat.com \
--cc=pl@kamp.de \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.