Peter, there is a question for you hidden somewhere below.
Am 24.10.2016 um 21:27 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.
Signed-off-by: Ashijeet Acharya <address@hidden>
---
block/nfs.c | 348 +++++++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 253 insertions(+), 95 deletions(-)
diff --git a/block/nfs.c b/block/nfs.c
index 8602a44..666ccf2 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,118 @@ 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;
+
+ 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) {
+ error_setg(errp, "missing hostname in URI");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (!uri->path) {
+ error_setg(errp, "missing file path in URI");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ 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));
Just style, but why the empty line between both qdict_put() calls?
+
+ 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;
You need to set ret = -EINVAL here.
+ }
+ 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;
And here.
+ }
+ 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;
And here.
If you prefer, you can set ret = -EINVAL at the top of the function and
do a ret = 0 only right before 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;
+ }
+
+ ret = nfs_parse_uri(filename, options, errp);
+ if (ret < 0) {
+ error_setg(errp, "No valid URL specified");
errp has already been set by nfs_parse_uri(), you must not set it a
second time. Otherwise, this is what you get:
$ x86_64-softmmu/qemu-system-x86_64 -drive file=nfs://foo
qemu-system-x86_64: util/error.c:57: error_setv: Assertion `*errp == ((void
*)0)' failed.
Aborted
+ }
+ return;
+}
+
static void nfs_process_read(void *arg);
static void nfs_process_write(void *arg);
@@ -228,15 +344,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 +429,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;
This is inconsistent. Either all error paths before initialising the
context should 'goto fail' anyway (and I'd prefer this), or they should
all 'goto out'. I don't like mixing both.
@@ -311,79 +476,77 @@ static int64_t nfs_client_open(NFSClient *client, const
char *filename,
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")) {
+ nfs_set_uid(client->context,
+ qemu_opt_get_number(opts, "uid", getuid()));
Isn't the getuid() useless here? We already check that an "uid" option
is passed (and I think doing that is correct), so the default is never
used. We could just as well pass 0 as the default.
+ }
+
+ if (qemu_opt_get(opts, "gid")) {
+ nfs_set_gid(client->context,
+ qemu_opt_get_number(opts, "gid", getgid()));
Here, too.
+ }
+
+ 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);
We have a functional change here. I'm not completely sure yet whether
you're fixing a bug here or you're just turning one bug into a different
bug, but the old code doesn't look right anyway.
If LIBNFS_FEATURE_READAHEAD was defined, but LIBNFS_FEATURE_PAGECACHE
wasn't, the nfs_set_pagecache_ttl() would end up in the "tcp-syncnt"
option handling, which is probably completely wrong.
I suspect that commit d99b26c4 added the second call accidentally
(intended just a copy and paste of the #ifdef line?) and this line
should simply be dropped.