* [Qemu-devel] [PATCH 0/2] allow blockdev-add for NFS
@ 2016-10-19 12:38 Ashijeet Acharya
2016-10-19 12:38 ` [Qemu-devel] [PATCH 1/2] block/nfs: Introduce runtime_opts in NFS Ashijeet Acharya
2016-10-19 12:38 ` [Qemu-devel] [PATCH 2/2] qapi: allow blockdev-add for NFS Ashijeet Acharya
0 siblings, 2 replies; 6+ messages in thread
From: Ashijeet Acharya @ 2016-10-19 12:38 UTC (permalink / raw)
To: pl
Cc: jcody, kwolf, mreitz, armbru, eblake, qemu-devel, qemu-block,
Ashijeet Acharya
This series adds blockdev-add support for NFS block driver.
Patch 1 helps to prepare NFS driver to make use of several runtime_opts
as they appear in the URI. This will make NFS to do things similar to
the way other drivers available in the block layer do.
Patch 2 helps to allow blockdev-add support for the NFS block driver
by making the NFS option available.
Ashijeet Acharya (2):
block/nfs: Introduce runtime_opts in NFS
qapi: allow blockdev-add for NFS
block/nfs.c | 360 +++++++++++++++++++++++++++++++++++++--------------
qapi/block-core.json | 56 +++++++-
2 files changed, 313 insertions(+), 103 deletions(-)
--
2.6.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 1/2] block/nfs: Introduce runtime_opts in NFS
2016-10-19 12:38 [Qemu-devel] [PATCH 0/2] allow blockdev-add for NFS Ashijeet Acharya
@ 2016-10-19 12:38 ` Ashijeet Acharya
2016-10-24 15:10 ` Kevin Wolf
2016-10-19 12:38 ` [Qemu-devel] [PATCH 2/2] qapi: allow blockdev-add for NFS Ashijeet Acharya
1 sibling, 1 reply; 6+ messages in thread
From: Ashijeet Acharya @ 2016-10-19 12:38 UTC (permalink / raw)
To: pl
Cc: jcody, kwolf, mreitz, armbru, eblake, qemu-devel, qemu-block,
Ashijeet Acharya
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) {
+ 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 : "/";
+ p += strspn(p, "/");
+ if (p[0]) {
+ qdict_put(options, "export", qstring_from_str(p));
+ }
+
+ 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, "://")) {
+ 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()));
}
- 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;
}
@@ -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,
--
2.6.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] qapi: allow blockdev-add for NFS
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-19 12:38 ` Ashijeet Acharya
1 sibling, 0 replies; 6+ messages in thread
From: Ashijeet Acharya @ 2016-10-19 12:38 UTC (permalink / raw)
To: pl
Cc: jcody, kwolf, mreitz, armbru, eblake, qemu-devel, qemu-block,
Ashijeet Acharya
Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to
support blockdev-add for NFS network protocol driver. Also make a new
struct NFSServer to support tcp connection.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 52 insertions(+), 4 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9d797b8..3ab028d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1714,9 +1714,9 @@
{ 'enum': 'BlockdevDriver',
'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
- 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
- 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
- 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
+ 'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio',
+ 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
+ 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
##
# @BlockdevOptionsFile
@@ -2212,6 +2212,54 @@
'*top-id': 'str' } }
##
+# @NFSServer
+#
+# Captures the address of the socket
+#
+# @type: transport type used for NFS (only TCP supported)
+#
+# @host: host part of the address
+#
+# Since 2.8
+##
+{ 'struct': 'NFSServer',
+ 'data': { 'type': 'str',
+ 'host': 'str' } }
+
+##
+# @BlockdevOptionsNfs
+#
+# Driver specific block device option for NFS
+#
+# @server: host address
+#
+# @path: path of the image on the host
+#
+# @uid: #optional UID value to use when talking to the server
+#
+# @gid: #optional GID value to use when talking to the server
+#
+# @tcp-syncnt: #optional number of SYNs during the session establishment
+#
+# @readahead: #optional set the readahead size in bytes
+#
+# @pagecache: #optional set the pagecache size in bytes
+#
+# @debug: #optional set the NFS debug level (max 2)
+#
+# Since 2.8
+##
+{ 'struct': 'BlockdevOptionsNfs',
+ 'data': { 'server': 'NFSServer',
+ 'path': 'str',
+ '*uid': 'int',
+ '*gid': 'int',
+ '*tcp-syncnt': 'int',
+ '*readahead': 'int',
+ '*pagecache': 'int',
+ '*debug': 'int' } }
+
+##
# @BlockdevOptionsCurl
#
# Driver specific block device options for the curl backend.
@@ -2269,7 +2317,7 @@
# TODO iscsi: Wait for structured options
'luks': 'BlockdevOptionsLUKS',
# TODO nbd: Should take InetSocketAddress for 'host'?
-# TODO nfs: Wait for structured options
+ 'nfs': 'BlockdevOptionsNfs',
'null-aio': 'BlockdevOptionsNull',
'null-co': 'BlockdevOptionsNull',
'parallels': 'BlockdevOptionsGenericFormat',
--
2.6.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] block/nfs: Introduce runtime_opts in NFS
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
2016-10-24 18:42 ` Ashijeet Acharya
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2016-10-24 15:10 UTC (permalink / raw)
To: Ashijeet Acharya
Cc: pl, jcody, mreitz, armbru, eblake, qemu-devel, qemu-block
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] block/nfs: Introduce runtime_opts in NFS
2016-10-24 15:10 ` Kevin Wolf
@ 2016-10-24 18:42 ` Ashijeet Acharya
2016-10-24 18:52 ` Ashijeet Acharya
0 siblings, 1 reply; 6+ messages in thread
From: Ashijeet Acharya @ 2016-10-24 18:42 UTC (permalink / raw)
To: Kevin Wolf
Cc: Peter Lieven, jcody, Max Reitz, Markus Armbruster, Eric Blake,
QEMU Developers, qemu-block
On Mon, Oct 24, 2016 at 8:40 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> 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.
Yes, fixed the same for uri->path too.
>
>> + 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.
I dropped "export" as we discussed on the IRC.
>
>> + 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.)
Exactly!! I follwed this chunk of code from NBD and the same doubt
came to my mind, but then I did not give it much thought. But since
you have noticed it, shouldn't the same logic apply to NBD?
>
>> + 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.
*sigh* Yes, I fixed this. Very careless mistake!!
>
>> - 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.
I realised this after discussing with you on the IRC and now I am
using 'url' to fill up the QDict options and then passing them to the
nfs_client_open() function.
>> @@ -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.
No problem. I will send v2 and await another round of review.
Ashijeet
>
> Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] block/nfs: Introduce runtime_opts in NFS
2016-10-24 18:42 ` Ashijeet Acharya
@ 2016-10-24 18:52 ` Ashijeet Acharya
0 siblings, 0 replies; 6+ messages in thread
From: Ashijeet Acharya @ 2016-10-24 18:52 UTC (permalink / raw)
To: Kevin Wolf
Cc: Peter Lieven, jcody, Max Reitz, Markus Armbruster, Eric Blake,
QEMU Developers, qemu-block
On Tue, Oct 25, 2016 at 12:12 AM, Ashijeet Acharya
<ashijeetacharya@gmail.com> wrote:
> On Mon, Oct 24, 2016 at 8:40 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> 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 @@
>>> + if (!uri->server || strcmp(uri->server, "") == 0) {
>>
>> No need to use strcmp(), !*uri->server is enough.
>
> Yes, fixed the same for uri->path too.
Can I ask why we do this check in SSH then?
Ashijeet
>> Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-24 18:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).