* [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 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.