From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38845) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bz2Jq-0002jn-MZ for qemu-devel@nongnu.org; Tue, 25 Oct 2016 10:03:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bz2Jm-00022h-MY for qemu-devel@nongnu.org; Tue, 25 Oct 2016 10:03:10 -0400 Date: Tue, 25 Oct 2016 16:02:48 +0200 From: Kevin Wolf Message-ID: <20161025140248.GI4695@noname.str.redhat.com> References: <1477337274-7939-1-git-send-email-ashijeetacharya@gmail.com> <1477337274-7939-2-git-send-email-ashijeetacharya@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1477337274-7939-2-git-send-email-ashijeetacharya@gmail.com> Subject: Re: [Qemu-devel] [PATCH v2 1/2] block/nfs: Introduce runtime_opts in NFS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ashijeet Acharya Cc: pl@kamp.de, jcody@redhat.com, mreitz@redhat.com, armbru@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org Peter, there is a question for you hidden somewhere below. Am 24.10.2016 um 21:27 hat Ashijeet Acharya geschrieben: > Make NFS block driver use various fine grained runtime_opts. > Set .bdrv_parse_filename() to nfs_parse_filename() and introduce two > new functions nfs_parse_filename() and nfs_parse_uri() to help parsing > the URI. > > Signed-off-by: Ashijeet Acharya > --- > block/nfs.c | 348 +++++++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 253 insertions(+), 95 deletions(-) > > diff --git a/block/nfs.c b/block/nfs.c > index 8602a44..666ccf2 100644 > --- a/block/nfs.c > +++ b/block/nfs.c > @@ -35,8 +35,12 @@ > #include "qemu/uri.h" > #include "qemu/cutils.h" > #include "sysemu/sysemu.h" > +#include "qapi/qmp/qdict.h" > +#include "qapi/qmp/qint.h" > +#include "qapi/qmp/qstring.h" > #include > > + > #define QEMU_NFS_MAX_READAHEAD_SIZE 1048576 > #define QEMU_NFS_MAX_PAGECACHE_SIZE (8388608 / NFS_BLKSIZE) > #define QEMU_NFS_MAX_DEBUG_LEVEL 2 > @@ -61,6 +65,118 @@ typedef struct NFSRPC { > NFSClient *client; > } NFSRPC; > > +static int nfs_parse_uri(const char *filename, QDict *options, Error **errp) > +{ > + URI *uri = NULL; > + QueryParams *qp = NULL; > + int ret = 0, i; > + > + uri = uri_parse(filename); > + if (!uri) { > + error_setg(errp, "Invalid URI specified"); > + ret = -EINVAL; > + goto out; > + } > + if (strcmp(uri->scheme, "nfs") != 0) { > + error_setg(errp, "URI scheme must be 'nfs'"); > + ret = -EINVAL; > + goto out; > + } > + > + if (!uri->server) { > + error_setg(errp, "missing hostname in URI"); > + ret = -EINVAL; > + goto out; > + } > + > + if (!uri->path) { > + error_setg(errp, "missing file path in URI"); > + ret = -EINVAL; > + goto out; > + } > + > + qp = query_params_parse(uri->query); > + if (!qp) { > + error_setg(errp, "could not parse query parameters"); > + ret = -EINVAL; > + goto out; > + } > + > + qdict_put(options, "host", qstring_from_str(uri->server)); > + > + qdict_put(options, "path", qstring_from_str(uri->path)); Just style, but why the empty line between both qdict_put() calls? > + > + for (i = 0; i < qp->n; i++) { > + if (!qp->p[i].value) { > + error_setg(errp, "Value for NFS parameter expected: %s", > + qp->p[i].name); > + goto out; You need to set ret = -EINVAL here. > + } > + if (parse_uint_full(qp->p[i].value, NULL, 0)) { > + error_setg(errp, "Illegal value for NFS parameter: %s", > + qp->p[i].name); > + goto out; And here. > + } > + if (!strcmp(qp->p[i].name, "uid")) { > + qdict_put(options, "uid", > + qstring_from_str(qp->p[i].value)); > + } else if (!strcmp(qp->p[i].name, "gid")) { > + qdict_put(options, "gid", > + qstring_from_str(qp->p[i].value)); > + } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) { > + qdict_put(options, "tcp-syncnt", > + qstring_from_str(qp->p[i].value)); > + } else if (!strcmp(qp->p[i].name, "readahead")) { > + qdict_put(options, "readahead", > + qstring_from_str(qp->p[i].value)); > + } else if (!strcmp(qp->p[i].name, "pagecache")) { > + qdict_put(options, "pagecache", > + qstring_from_str(qp->p[i].value)); > + } else if (!strcmp(qp->p[i].name, "debug")) { > + qdict_put(options, "debug", > + qstring_from_str(qp->p[i].value)); > + } else { > + error_setg(errp, "Unknown NFS parameter name: %s", > + qp->p[i].name); > + goto out; And here. If you prefer, you can set ret = -EINVAL at the top of the function and do a ret = 0 only right before out: > + } > + } > +out: > + if (qp) { > + query_params_free(qp); > + } > + if (uri) { > + uri_free(uri); > + } > + return ret; > +} > + > +static void nfs_parse_filename(const char *filename, QDict *options, > + Error **errp) > +{ > + int ret = 0; > + > + if (qdict_haskey(options, "host") || > + qdict_haskey(options, "path") || > + qdict_haskey(options, "uid") || > + qdict_haskey(options, "gid") || > + qdict_haskey(options, "tcp-syncnt") || > + qdict_haskey(options, "readahead") || > + qdict_haskey(options, "pagecache") || > + qdict_haskey(options, "debug")) { > + error_setg(errp, "host/path/uid/gid/tcp-syncnt/readahead/pagecache" > + "/debug and a filename may not be used at the same" > + " time"); > + return; > + } > + > + ret = nfs_parse_uri(filename, options, errp); > + if (ret < 0) { > + error_setg(errp, "No valid URL specified"); errp has already been set by nfs_parse_uri(), you must not set it a second time. Otherwise, this is what you get: $ x86_64-softmmu/qemu-system-x86_64 -drive file=nfs://foo qemu-system-x86_64: util/error.c:57: error_setv: Assertion `*errp == ((void *)0)' failed. Aborted > + } > + return; > +} > + > static void nfs_process_read(void *arg); > static void nfs_process_write(void *arg); > > @@ -228,15 +344,49 @@ static int coroutine_fn nfs_co_flush(BlockDriverState *bs) > return task.ret; > } > > -/* TODO Convert to fine grained options */ > static QemuOptsList runtime_opts = { > .name = "nfs", > .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head), > .desc = { > { > - .name = "filename", > + .name = "host", > + .type = QEMU_OPT_STRING, > + .help = "Host to connect to", > + }, > + { > + .name = "path", > .type = QEMU_OPT_STRING, > - .help = "URL to the NFS file", > + .help = "Path of the image on the host", > + }, > + { > + .name = "uid", > + .type = QEMU_OPT_NUMBER, > + .help = "UID value to use when talking to the server", > + }, > + { > + .name = "gid", > + .type = QEMU_OPT_NUMBER, > + .help = "GID value to use when talking to the server", > + }, > + { > + .name = "tcp-syncnt", > + .type = QEMU_OPT_NUMBER, > + .help = "Number of SYNs to send during the session establish", > + }, > + { > + .name = "readahead", > + .type = QEMU_OPT_NUMBER, > + .help = "Set the readahead size in bytes", > + }, > + { > + .name = "pagecache", > + .type = QEMU_OPT_NUMBER, > + .help = "Set the pagecache size in bytes", > + }, > + { > + .name = "debug", > + .type = QEMU_OPT_NUMBER, > + .help = "Set the NFS debug level (max 2)", > }, > { /* end of list */ } > }, > @@ -279,25 +429,40 @@ static void nfs_file_close(BlockDriverState *bs) > nfs_client_close(client); > } > > -static int64_t nfs_client_open(NFSClient *client, const char *filename, > +static int64_t nfs_client_open(NFSClient *client, QDict *options, > int flags, Error **errp, int open_flags) > { > - int ret = -EINVAL, i; > + int ret = -EINVAL; > + QemuOpts *opts = NULL; > + Error *local_err = NULL; > struct stat st; > - URI *uri; > - QueryParams *qp = NULL; > char *file = NULL, *strp = NULL; > + const char *host, *path; > + unsigned long long val; > > - uri = uri_parse(filename); > - if (!uri) { > - error_setg(errp, "Invalid URL specified"); > - goto fail; > + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); > + qemu_opts_absorb_qdict(opts, options, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + ret = -EINVAL; > + goto out; > } > - if (!uri->server) { > - error_setg(errp, "Invalid URL specified"); > - goto fail; > + > + host = qemu_opt_get(opts, "host"); > + if (!host) { > + ret = -EINVAL; > + error_setg(errp, "No hostname was specified"); > + goto out; > } > - strp = strrchr(uri->path, '/'); > + > + path = qemu_opt_get(opts, "path"); > + if (!path) { > + ret = -EINVAL; > + error_setg(errp, "No path was specified"); > + goto out; > + } > + > + strp = strrchr(path, '/'); > if (strp == NULL) { > error_setg(errp, "Invalid URL specified"); > goto fail; This is inconsistent. Either all error paths before initialising the context should 'goto fail' anyway (and I'd prefer this), or they should all 'goto out'. I don't like mixing both. > @@ -311,79 +476,77 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename, > goto fail; > } > > - qp = query_params_parse(uri->query); > - for (i = 0; i < qp->n; i++) { > - unsigned long long val; > - if (!qp->p[i].value) { > - error_setg(errp, "Value for NFS parameter expected: %s", > - qp->p[i].name); > + if (qemu_opt_get(opts, "uid")) { > + nfs_set_uid(client->context, > + qemu_opt_get_number(opts, "uid", getuid())); Isn't the getuid() useless here? We already check that an "uid" option is passed (and I think doing that is correct), so the default is never used. We could just as well pass 0 as the default. > + } > + > + if (qemu_opt_get(opts, "gid")) { > + nfs_set_gid(client->context, > + qemu_opt_get_number(opts, "gid", getgid())); Here, too. > + } > + > + if (qemu_opt_get(opts, "tcp-syncnt")) { > + nfs_set_tcp_syncnt(client->context, > + qemu_opt_get_number(opts, "tcp-syncnt", 0)); > + } > + > +#ifdef LIBNFS_FEATURE_READAHEAD > + if (qemu_opt_get(opts, "readahead")) { > + if (open_flags & BDRV_O_NOCACHE) { > + error_setg(errp, "Cannot enable NFS readahead " > + "if cache.direct = on"); > goto fail; > } > - if (parse_uint_full(qp->p[i].value, &val, 0)) { > - error_setg(errp, "Illegal value for NFS parameter: %s", > - qp->p[i].name); > - goto fail; > + val = qemu_opt_get_number(opts, "readahead", 0); > + if (val > QEMU_NFS_MAX_READAHEAD_SIZE) { > + error_report("NFS Warning: Truncating NFS readahead " > + "size to %d", QEMU_NFS_MAX_READAHEAD_SIZE); > + val = QEMU_NFS_MAX_READAHEAD_SIZE; > } > - if (!strcmp(qp->p[i].name, "uid")) { > - nfs_set_uid(client->context, val); > - } else if (!strcmp(qp->p[i].name, "gid")) { > - nfs_set_gid(client->context, val); > - } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) { > - nfs_set_tcp_syncnt(client->context, val); > -#ifdef LIBNFS_FEATURE_READAHEAD > - } else if (!strcmp(qp->p[i].name, "readahead")) { > - if (open_flags & BDRV_O_NOCACHE) { > - error_setg(errp, "Cannot enable NFS readahead " > - "if cache.direct = on"); > - goto fail; > - } > - if (val > QEMU_NFS_MAX_READAHEAD_SIZE) { > - error_report("NFS Warning: Truncating NFS readahead" > - " size to %d", QEMU_NFS_MAX_READAHEAD_SIZE); > - val = QEMU_NFS_MAX_READAHEAD_SIZE; > - } > - nfs_set_readahead(client->context, val); > + nfs_set_readahead(client->context, val); > #ifdef LIBNFS_FEATURE_PAGECACHE > - nfs_set_pagecache_ttl(client->context, 0); > + nfs_set_pagecache_ttl(client->context, 0); > #endif > - client->cache_used = true; > + client->cache_used = true; > + } > #endif > + > #ifdef LIBNFS_FEATURE_PAGECACHE > - nfs_set_pagecache_ttl(client->context, 0); > - } else if (!strcmp(qp->p[i].name, "pagecache")) { > - if (open_flags & BDRV_O_NOCACHE) { > - error_setg(errp, "Cannot enable NFS pagecache " > - "if cache.direct = on"); > - goto fail; > - } > - if (val > QEMU_NFS_MAX_PAGECACHE_SIZE) { > - error_report("NFS Warning: Truncating NFS pagecache" > - " size to %d pages", QEMU_NFS_MAX_PAGECACHE_SIZE); > - val = QEMU_NFS_MAX_PAGECACHE_SIZE; > - } > - nfs_set_pagecache(client->context, val); > - nfs_set_pagecache_ttl(client->context, 0); > - client->cache_used = true; > + nfs_set_pagecache_ttl(client->context, 0); We have a functional change here. I'm not completely sure yet whether you're fixing a bug here or you're just turning one bug into a different bug, but the old code doesn't look right anyway. If LIBNFS_FEATURE_READAHEAD was defined, but LIBNFS_FEATURE_PAGECACHE wasn't, the nfs_set_pagecache_ttl() would end up in the "tcp-syncnt" option handling, which is probably completely wrong. I suspect that commit d99b26c4 added the second call accidentally (intended just a copy and paste of the #ifdef line?) and this line should simply be dropped. Peter? > + 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 > > - 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 +580,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 +593,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 +625,7 @@ static int nfs_file_create(const char *url, QemuOpts *opts, Error **errp) > int ret = 0; > int64_t total_size = 0; > NFSClient *client = g_new0(NFSClient, 1); > + QDict *options = NULL; > > client->aio_context = qemu_get_aio_context(); > > @@ -482,7 +633,14 @@ static int nfs_file_create(const char *url, QemuOpts *opts, Error **errp) > total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), > BDRV_SECTOR_SIZE); > > - ret = nfs_client_open(client, url, O_CREAT, errp, 0); > + options = qdict_new(); > + ret = nfs_parse_uri(url, options, errp); > + if (ret < 0) { > + error_setg(errp, "No valid URL specified"); Same thing as above, you can't set errp twice. > + goto out; > + } > + > + ret = nfs_client_open(client, options, O_CREAT, errp, 0); > if (ret < 0) { > goto out; > } > @@ -578,7 +736,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, Kevin