| Message ID | 1477337274-7939-2-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <ashijeetacharya@gmail.com> > --- > block/nfs.c | 348 +++++++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 253 insertions(+), 95 deletions(-) > > diff --git a/block/nfs.c b/block/nfs.c > index 8602a44..666ccf2 100644 > --- a/block/nfs.c > +++ b/block/nfs.c > @@ -35,8 +35,12 @@ > #include "qemu/uri.h" > #include "qemu/cutils.h" > #include "sysemu/sysemu.h" > +#include "qapi/qmp/qdict.h" > +#include "qapi/qmp/qint.h" > +#include "qapi/qmp/qstring.h" > #include <nfsc/libnfs.h> > > + > #define QEMU_NFS_MAX_READAHEAD_SIZE 1048576 > #define QEMU_NFS_MAX_PAGECACHE_SIZE (8388608 / NFS_BLKSIZE) > #define QEMU_NFS_MAX_DEBUG_LEVEL 2 > @@ -61,6 +65,118 @@ typedef struct NFSRPC { > NFSClient *client; > } NFSRPC; > > +static int nfs_parse_uri(const char *filename, QDict *options, Error **errp) > +{ > + URI *uri = NULL; > + QueryParams *qp = NULL; > + int ret = 0, i; > + > + uri = uri_parse(filename); > + if (!uri) { > + error_setg(errp, "Invalid URI specified"); > + ret = -EINVAL; > + goto out; > + } > + if (strcmp(uri->scheme, "nfs") != 0) { > + error_setg(errp, "URI scheme must be 'nfs'"); > + ret = -EINVAL; > + goto out; > + } > + > + if (!uri->server) { > + error_setg(errp, "missing hostname in URI"); > + ret = -EINVAL; > + goto out; > + } > + > + if (!uri->path) { > + error_setg(errp, "missing file path in URI"); > + ret = -EINVAL; > + goto out; > + } > + > + qp = query_params_parse(uri->query); > + if (!qp) { > + error_setg(errp, "could not parse query parameters"); > + ret = -EINVAL; > + goto out; > + } > + > + qdict_put(options, "host", qstring_from_str(uri->server)); > + > + qdict_put(options, "path", qstring_from_str(uri->path)); Just style, but why the empty line between both qdict_put() calls? > + > + for (i = 0; i < qp->n; i++) { > + if (!qp->p[i].value) { > + error_setg(errp, "Value for NFS parameter expected: %s", > + qp->p[i].name); > + goto out; You need to set ret = -EINVAL here. > + } > + if (parse_uint_full(qp->p[i].value, NULL, 0)) { > + error_setg(errp, "Illegal value for NFS parameter: %s", > + qp->p[i].name); > + goto out; And here. > + } > + if (!strcmp(qp->p[i].name, "uid")) { > + qdict_put(options, "uid", > + qstring_from_str(qp->p[i].value)); > + } else if (!strcmp(qp->p[i].name, "gid")) { > + qdict_put(options, "gid", > + qstring_from_str(qp->p[i].value)); > + } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) { > + qdict_put(options, "tcp-syncnt", > + qstring_from_str(qp->p[i].value)); > + } else if (!strcmp(qp->p[i].name, "readahead")) { > + qdict_put(options, "readahead", > + qstring_from_str(qp->p[i].value)); > + } else if (!strcmp(qp->p[i].name, "pagecache")) { > + qdict_put(options, "pagecache", > + qstring_from_str(qp->p[i].value)); > + } else if (!strcmp(qp->p[i].name, "debug")) { > + qdict_put(options, "debug", > + qstring_from_str(qp->p[i].value)); > + } else { > + error_setg(errp, "Unknown NFS parameter name: %s", > + qp->p[i].name); > + goto out; And here. If you prefer, you can set ret = -EINVAL at the top of the function and do a ret = 0 only right before out: > + } > + } > +out: > + if (qp) { > + query_params_free(qp); > + } > + if (uri) { > + uri_free(uri); > + } > + return ret; > +} > + > +static void nfs_parse_filename(const char *filename, QDict *options, > + Error **errp) > +{ > + int ret = 0; > + > + if (qdict_haskey(options, "host") || > + qdict_haskey(options, "path") || > + qdict_haskey(options, "uid") || > + qdict_haskey(options, "gid") || > + qdict_haskey(options, "tcp-syncnt") || > + qdict_haskey(options, "readahead") || > + qdict_haskey(options, "pagecache") || > + qdict_haskey(options, "debug")) { > + error_setg(errp, "host/path/uid/gid/tcp-syncnt/readahead/pagecache" > + "/debug and a filename may not be used at the same" > + " time"); > + return; > + } > + > + ret = nfs_parse_uri(filename, options, errp); > + if (ret < 0) { > + error_setg(errp, "No valid URL specified"); errp has already been set by nfs_parse_uri(), you must not set it a second time. Otherwise, this is what you get: $ x86_64-softmmu/qemu-system-x86_64 -drive file=nfs://foo qemu-system-x86_64: util/error.c:57: error_setv: Assertion `*errp == ((void *)0)' failed. Aborted > + } > + return; > +} > + > static void nfs_process_read(void *arg); > static void nfs_process_write(void *arg); > > @@ -228,15 +344,49 @@ static int coroutine_fn nfs_co_flush(BlockDriverState *bs) > return task.ret; > } > > -/* TODO Convert to fine grained options */ > static QemuOptsList runtime_opts = { > .name = "nfs", > .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head), > .desc = { > { > - .name = "filename", > + .name = "host", > + .type = QEMU_OPT_STRING, > + .help = "Host to connect to", > + }, > + { > + .name = "path", > .type = QEMU_OPT_STRING, > - .help = "URL to the NFS file", > + .help = "Path of the image on the host", > + }, > + { > + .name = "uid", > + .type = QEMU_OPT_NUMBER, > + .help = "UID value to use when talking to the server", > + }, > + { > + .name = "gid", > + .type = QEMU_OPT_NUMBER, > + .help = "GID value to use when talking to the server", > + }, > + { > + .name = "tcp-syncnt", > + .type = QEMU_OPT_NUMBER, > + .help = "Number of SYNs to send during the session establish", > + }, > + { > + .name = "readahead", > + .type = QEMU_OPT_NUMBER, > + .help = "Set the readahead size in bytes", > + }, > + { > + .name = "pagecache", > + .type = QEMU_OPT_NUMBER, > + .help = "Set the pagecache size in bytes", > + }, > + { > + .name = "debug", > + .type = QEMU_OPT_NUMBER, > + .help = "Set the NFS debug level (max 2)", > }, > { /* end of list */ } > }, > @@ -279,25 +429,40 @@ static void nfs_file_close(BlockDriverState *bs) > nfs_client_close(client); > } > > -static int64_t nfs_client_open(NFSClient *client, const char *filename, > +static int64_t nfs_client_open(NFSClient *client, QDict *options, > int flags, Error **errp, int open_flags) > { > - int ret = -EINVAL, i; > + int ret = -EINVAL; > + QemuOpts *opts = NULL; > + Error *local_err = NULL; > struct stat st; > - URI *uri; > - QueryParams *qp = NULL; > char *file = NULL, *strp = NULL; > + const char *host, *path; > + unsigned long long val; > > - uri = uri_parse(filename); > - if (!uri) { > - error_setg(errp, "Invalid URL specified"); > - goto fail; > + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); > + qemu_opts_absorb_qdict(opts, options, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + ret = -EINVAL; > + goto out; > } > - if (!uri->server) { > - error_setg(errp, "Invalid URL specified"); > - goto fail; > + > + host = qemu_opt_get(opts, "host"); > + if (!host) { > + ret = -EINVAL; > + error_setg(errp, "No hostname was specified"); > + goto out; > } > - strp = strrchr(uri->path, '/'); > + > + path = qemu_opt_get(opts, "path"); > + if (!path) { > + ret = -EINVAL; > + error_setg(errp, "No path was specified"); > + goto out; > + } > + > + strp = strrchr(path, '/'); > if (strp == NULL) { > error_setg(errp, "Invalid URL specified"); > goto fail; This is inconsistent. Either all error paths before initialising the context should 'goto fail' anyway (and I'd prefer this), or they should all 'goto out'. I don't like mixing both. > @@ -311,79 +476,77 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename, > goto fail; > } > > - qp = query_params_parse(uri->query); > - for (i = 0; i < qp->n; i++) { > - unsigned long long val; > - if (!qp->p[i].value) { > - error_setg(errp, "Value for NFS parameter expected: %s", > - qp->p[i].name); > + if (qemu_opt_get(opts, "uid")) { > + nfs_set_uid(client->context, > + qemu_opt_get_number(opts, "uid", getuid())); Isn't the getuid() useless here? We already check that an "uid" option is passed (and I think doing that is correct), so the default is never used. We could just as well pass 0 as the default. > + } > + > + if (qemu_opt_get(opts, "gid")) { > + nfs_set_gid(client->context, > + qemu_opt_get_number(opts, "gid", getgid())); Here, too. > + } > + > + if (qemu_opt_get(opts, "tcp-syncnt")) { > + nfs_set_tcp_syncnt(client->context, > + qemu_opt_get_number(opts, "tcp-syncnt", 0)); > + } > + > +#ifdef LIBNFS_FEATURE_READAHEAD > + if (qemu_opt_get(opts, "readahead")) { > + if (open_flags & BDRV_O_NOCACHE) { > + error_setg(errp, "Cannot enable NFS readahead " > + "if cache.direct = on"); > goto fail; > } > - if (parse_uint_full(qp->p[i].value, &val, 0)) { > - error_setg(errp, "Illegal value for NFS parameter: %s", > - qp->p[i].name); > - goto fail; > + val = qemu_opt_get_number(opts, "readahead", 0); > + if (val > QEMU_NFS_MAX_READAHEAD_SIZE) { > + error_report("NFS Warning: Truncating NFS readahead " > + "size to %d", QEMU_NFS_MAX_READAHEAD_SIZE); > + val = QEMU_NFS_MAX_READAHEAD_SIZE; > } > - if (!strcmp(qp->p[i].name, "uid")) { > - nfs_set_uid(client->context, val); > - } else if (!strcmp(qp->p[i].name, "gid")) { > - nfs_set_gid(client->context, val); > - } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) { > - nfs_set_tcp_syncnt(client->context, val); > -#ifdef LIBNFS_FEATURE_READAHEAD > - } else if (!strcmp(qp->p[i].name, "readahead")) { > - if (open_flags & BDRV_O_NOCACHE) { > - error_setg(errp, "Cannot enable NFS readahead " > - "if cache.direct = on"); > - goto fail; > - } > - if (val > QEMU_NFS_MAX_READAHEAD_SIZE) { > - error_report("NFS Warning: Truncating NFS readahead" > - " size to %d", QEMU_NFS_MAX_READAHEAD_SIZE); > - val = QEMU_NFS_MAX_READAHEAD_SIZE; > - } > - nfs_set_readahead(client->context, val); > + nfs_set_readahead(client->context, val); > #ifdef LIBNFS_FEATURE_PAGECACHE > - nfs_set_pagecache_ttl(client->context, 0); > + nfs_set_pagecache_ttl(client->context, 0); > #endif > - client->cache_used = true; > + client->cache_used = true; > + } > #endif > + > #ifdef LIBNFS_FEATURE_PAGECACHE > - nfs_set_pagecache_ttl(client->context, 0); > - } else if (!strcmp(qp->p[i].name, "pagecache")) { > - if (open_flags & BDRV_O_NOCACHE) { > - error_setg(errp, "Cannot enable NFS pagecache " > - "if cache.direct = on"); > - goto fail; > - } > - if (val > QEMU_NFS_MAX_PAGECACHE_SIZE) { > - error_report("NFS Warning: Truncating NFS pagecache" > - " size to %d pages", QEMU_NFS_MAX_PAGECACHE_SIZE); > - val = QEMU_NFS_MAX_PAGECACHE_SIZE; > - } > - nfs_set_pagecache(client->context, val); > - nfs_set_pagecache_ttl(client->context, 0); > - client->cache_used = true; > + nfs_set_pagecache_ttl(client->context, 0); We have a functional change here. I'm not completely sure yet whether you're fixing a bug here or you're just turning one bug into a different bug, but the old code doesn't look right anyway. If LIBNFS_FEATURE_READAHEAD was defined, but LIBNFS_FEATURE_PAGECACHE wasn't, the nfs_set_pagecache_ttl() would end up in the "tcp-syncnt" option handling, which is probably completely wrong. I suspect that commit d99b26c4 added the second call accidentally (intended just a copy and paste of the #ifdef line?) and this line should simply be dropped. 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
Am 25.10.2016 um 16:02 schrieb Kevin Wolf: > 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 <ashijeetacharya@gmail.com> >> --- >> block/nfs.c | 348 +++++++++++++++++++++++++++++++++++++++++++----------------- >> 1 file changed, 253 insertions(+), 95 deletions(-) >> >> diff --git a/block/nfs.c b/block/nfs.c >> index 8602a44..666ccf2 100644 >> --- a/block/nfs.c >> +++ b/block/nfs.c >> @@ -35,8 +35,12 @@ >> #include "qemu/uri.h" >> #include "qemu/cutils.h" >> #include "sysemu/sysemu.h" >> +#include "qapi/qmp/qdict.h" >> +#include "qapi/qmp/qint.h" >> +#include "qapi/qmp/qstring.h" >> #include <nfsc/libnfs.h> >> >> + >> #define QEMU_NFS_MAX_READAHEAD_SIZE 1048576 >> #define QEMU_NFS_MAX_PAGECACHE_SIZE (8388608 / NFS_BLKSIZE) >> #define QEMU_NFS_MAX_DEBUG_LEVEL 2 >> @@ -61,6 +65,118 @@ typedef struct NFSRPC { >> NFSClient *client; >> } NFSRPC; >> >> +static int nfs_parse_uri(const char *filename, QDict *options, Error **errp) >> +{ >> + URI *uri = NULL; >> + QueryParams *qp = NULL; >> + int ret = 0, i; >> + >> + uri = uri_parse(filename); >> + if (!uri) { >> + error_setg(errp, "Invalid URI specified"); >> + ret = -EINVAL; >> + goto out; >> + } >> + if (strcmp(uri->scheme, "nfs") != 0) { >> + error_setg(errp, "URI scheme must be 'nfs'"); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + if (!uri->server) { >> + error_setg(errp, "missing hostname in URI"); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + if (!uri->path) { >> + error_setg(errp, "missing file path in URI"); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + qp = query_params_parse(uri->query); >> + if (!qp) { >> + error_setg(errp, "could not parse query parameters"); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + qdict_put(options, "host", qstring_from_str(uri->server)); >> + >> + qdict_put(options, "path", qstring_from_str(uri->path)); > Just style, but why the empty line between both qdict_put() calls? > >> + >> + for (i = 0; i < qp->n; i++) { >> + if (!qp->p[i].value) { >> + error_setg(errp, "Value for NFS parameter expected: %s", >> + qp->p[i].name); >> + goto out; > You need to set ret = -EINVAL here. > >> + } >> + if (parse_uint_full(qp->p[i].value, NULL, 0)) { >> + error_setg(errp, "Illegal value for NFS parameter: %s", >> + qp->p[i].name); >> + goto out; > And here. > >> + } >> + if (!strcmp(qp->p[i].name, "uid")) { >> + qdict_put(options, "uid", >> + qstring_from_str(qp->p[i].value)); >> + } else if (!strcmp(qp->p[i].name, "gid")) { >> + qdict_put(options, "gid", >> + qstring_from_str(qp->p[i].value)); >> + } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) { >> + qdict_put(options, "tcp-syncnt", >> + qstring_from_str(qp->p[i].value)); >> + } else if (!strcmp(qp->p[i].name, "readahead")) { >> + qdict_put(options, "readahead", >> + qstring_from_str(qp->p[i].value)); >> + } else if (!strcmp(qp->p[i].name, "pagecache")) { >> + qdict_put(options, "pagecache", >> + qstring_from_str(qp->p[i].value)); >> + } else if (!strcmp(qp->p[i].name, "debug")) { >> + qdict_put(options, "debug", >> + qstring_from_str(qp->p[i].value)); >> + } else { >> + error_setg(errp, "Unknown NFS parameter name: %s", >> + qp->p[i].name); >> + goto out; > And here. > > If you prefer, you can set ret = -EINVAL at the top of the function and > do a ret = 0 only right before out: > >> + } >> + } >> +out: >> + if (qp) { >> + query_params_free(qp); >> + } >> + if (uri) { >> + uri_free(uri); >> + } >> + return ret; >> +} >> + >> +static void nfs_parse_filename(const char *filename, QDict *options, >> + Error **errp) >> +{ >> + int ret = 0; >> + >> + if (qdict_haskey(options, "host") || >> + qdict_haskey(options, "path") || >> + qdict_haskey(options, "uid") || >> + qdict_haskey(options, "gid") || >> + qdict_haskey(options, "tcp-syncnt") || >> + qdict_haskey(options, "readahead") || >> + qdict_haskey(options, "pagecache") || >> + qdict_haskey(options, "debug")) { >> + error_setg(errp, "host/path/uid/gid/tcp-syncnt/readahead/pagecache" >> + "/debug and a filename may not be used at the same" >> + " time"); >> + return; >> + } >> + >> + ret = nfs_parse_uri(filename, options, errp); >> + if (ret < 0) { >> + error_setg(errp, "No valid URL specified"); > errp has already been set by nfs_parse_uri(), you must not set it a > second time. Otherwise, this is what you get: > > $ x86_64-softmmu/qemu-system-x86_64 -drive file=nfs://foo > qemu-system-x86_64: util/error.c:57: error_setv: Assertion `*errp == ((void *)0)' failed. > Aborted > >> + } >> + return; >> +} >> + >> static void nfs_process_read(void *arg); >> static void nfs_process_write(void *arg); >> >> @@ -228,15 +344,49 @@ static int coroutine_fn nfs_co_flush(BlockDriverState *bs) >> return task.ret; >> } >> >> -/* TODO Convert to fine grained options */ >> static QemuOptsList runtime_opts = { >> .name = "nfs", >> .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head), >> .desc = { >> { >> - .name = "filename", >> + .name = "host", >> + .type = QEMU_OPT_STRING, >> + .help = "Host to connect to", >> + }, >> + { >> + .name = "path", >> .type = QEMU_OPT_STRING, >> - .help = "URL to the NFS file", >> + .help = "Path of the image on the host", >> + }, >> + { >> + .name = "uid", >> + .type = QEMU_OPT_NUMBER, >> + .help = "UID value to use when talking to the server", >> + }, >> + { >> + .name = "gid", >> + .type = QEMU_OPT_NUMBER, >> + .help = "GID value to use when talking to the server", >> + }, >> + { >> + .name = "tcp-syncnt", >> + .type = QEMU_OPT_NUMBER, >> + .help = "Number of SYNs to send during the session establish", >> + }, >> + { >> + .name = "readahead", >> + .type = QEMU_OPT_NUMBER, >> + .help = "Set the readahead size in bytes", >> + }, >> + { >> + .name = "pagecache", >> + .type = QEMU_OPT_NUMBER, >> + .help = "Set the pagecache size in bytes", >> + }, >> + { >> + .name = "debug", >> + .type = QEMU_OPT_NUMBER, >> + .help = "Set the NFS debug level (max 2)", >> }, >> { /* end of list */ } >> }, >> @@ -279,25 +429,40 @@ static void nfs_file_close(BlockDriverState *bs) >> nfs_client_close(client); >> } >> >> -static int64_t nfs_client_open(NFSClient *client, const char *filename, >> +static int64_t nfs_client_open(NFSClient *client, QDict *options, >> int flags, Error **errp, int open_flags) >> { >> - int ret = -EINVAL, i; >> + int ret = -EINVAL; >> + QemuOpts *opts = NULL; >> + Error *local_err = NULL; >> struct stat st; >> - URI *uri; >> - QueryParams *qp = NULL; >> char *file = NULL, *strp = NULL; >> + const char *host, *path; >> + unsigned long long val; >> >> - uri = uri_parse(filename); >> - if (!uri) { >> - error_setg(errp, "Invalid URL specified"); >> - goto fail; >> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); >> + qemu_opts_absorb_qdict(opts, options, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + ret = -EINVAL; >> + goto out; >> } >> - if (!uri->server) { >> - error_setg(errp, "Invalid URL specified"); >> - goto fail; >> + >> + host = qemu_opt_get(opts, "host"); >> + if (!host) { >> + ret = -EINVAL; >> + error_setg(errp, "No hostname was specified"); >> + goto out; >> } >> - strp = strrchr(uri->path, '/'); >> + >> + path = qemu_opt_get(opts, "path"); >> + if (!path) { >> + ret = -EINVAL; >> + error_setg(errp, "No path was specified"); >> + goto out; >> + } >> + >> + strp = strrchr(path, '/'); >> if (strp == NULL) { >> error_setg(errp, "Invalid URL specified"); >> goto fail; > This is inconsistent. Either all error paths before initialising the > context should 'goto fail' anyway (and I'd prefer this), or they should > all 'goto out'. I don't like mixing both. > >> @@ -311,79 +476,77 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename, >> goto fail; >> } >> >> - qp = query_params_parse(uri->query); >> - for (i = 0; i < qp->n; i++) { >> - unsigned long long val; >> - if (!qp->p[i].value) { >> - error_setg(errp, "Value for NFS parameter expected: %s", >> - qp->p[i].name); >> + if (qemu_opt_get(opts, "uid")) { >> + nfs_set_uid(client->context, >> + qemu_opt_get_number(opts, "uid", getuid())); > Isn't the getuid() useless here? We already check that an "uid" option > is passed (and I think doing that is correct), so the default is never > used. We could just as well pass 0 as the default. > >> + } >> + >> + if (qemu_opt_get(opts, "gid")) { >> + nfs_set_gid(client->context, >> + qemu_opt_get_number(opts, "gid", getgid())); > Here, too. > >> + } >> + >> + if (qemu_opt_get(opts, "tcp-syncnt")) { >> + nfs_set_tcp_syncnt(client->context, >> + qemu_opt_get_number(opts, "tcp-syncnt", 0)); >> + } >> + >> +#ifdef LIBNFS_FEATURE_READAHEAD >> + if (qemu_opt_get(opts, "readahead")) { >> + if (open_flags & BDRV_O_NOCACHE) { >> + error_setg(errp, "Cannot enable NFS readahead " >> + "if cache.direct = on"); >> goto fail; >> } >> - if (parse_uint_full(qp->p[i].value, &val, 0)) { >> - error_setg(errp, "Illegal value for NFS parameter: %s", >> - qp->p[i].name); >> - goto fail; >> + val = qemu_opt_get_number(opts, "readahead", 0); >> + if (val > QEMU_NFS_MAX_READAHEAD_SIZE) { >> + error_report("NFS Warning: Truncating NFS readahead " >> + "size to %d", QEMU_NFS_MAX_READAHEAD_SIZE); >> + val = QEMU_NFS_MAX_READAHEAD_SIZE; >> } >> - if (!strcmp(qp->p[i].name, "uid")) { >> - nfs_set_uid(client->context, val); >> - } else if (!strcmp(qp->p[i].name, "gid")) { >> - nfs_set_gid(client->context, val); >> - } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) { >> - nfs_set_tcp_syncnt(client->context, val); >> -#ifdef LIBNFS_FEATURE_READAHEAD >> - } else if (!strcmp(qp->p[i].name, "readahead")) { >> - if (open_flags & BDRV_O_NOCACHE) { >> - error_setg(errp, "Cannot enable NFS readahead " >> - "if cache.direct = on"); >> - goto fail; >> - } >> - if (val > QEMU_NFS_MAX_READAHEAD_SIZE) { >> - error_report("NFS Warning: Truncating NFS readahead" >> - " size to %d", QEMU_NFS_MAX_READAHEAD_SIZE); >> - val = QEMU_NFS_MAX_READAHEAD_SIZE; >> - } >> - nfs_set_readahead(client->context, val); >> + nfs_set_readahead(client->context, val); >> #ifdef LIBNFS_FEATURE_PAGECACHE >> - nfs_set_pagecache_ttl(client->context, 0); >> + nfs_set_pagecache_ttl(client->context, 0); >> #endif >> - client->cache_used = true; >> + client->cache_used = true; >> + } >> #endif >> + >> #ifdef LIBNFS_FEATURE_PAGECACHE >> - nfs_set_pagecache_ttl(client->context, 0); >> - } else if (!strcmp(qp->p[i].name, "pagecache")) { >> - if (open_flags & BDRV_O_NOCACHE) { >> - error_setg(errp, "Cannot enable NFS pagecache " >> - "if cache.direct = on"); >> - goto fail; >> - } >> - if (val > QEMU_NFS_MAX_PAGECACHE_SIZE) { >> - error_report("NFS Warning: Truncating NFS pagecache" >> - " size to %d pages", QEMU_NFS_MAX_PAGECACHE_SIZE); >> - val = QEMU_NFS_MAX_PAGECACHE_SIZE; >> - } >> - nfs_set_pagecache(client->context, val); >> - nfs_set_pagecache_ttl(client->context, 0); >> - client->cache_used = true; >> + nfs_set_pagecache_ttl(client->context, 0); > We have a functional change here. I'm not completely sure yet whether > you're fixing a bug here or you're just turning one bug into a different > bug, but the old code doesn't look right anyway. > > If LIBNFS_FEATURE_READAHEAD was defined, but LIBNFS_FEATURE_PAGECACHE > wasn't, the nfs_set_pagecache_ttl() would end up in the "tcp-syncnt" > option handling, which is probably completely wrong. > > I suspect that commit d99b26c4 added the second call accidentally > (intended just a copy and paste of the #ifdef line?) and this line > should simply be dropped. That line is accidently duplicated, but it has no negative impact since if LIBNFS_FEATURE_PAGECACHE is defined, LIBNFS_FEATURE_READAHEAD must be as well. Peter
diff --git a/block/nfs.c b/block/nfs.c index 8602a44..666ccf2 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -35,8 +35,12 @@ #include "qemu/uri.h" #include "qemu/cutils.h" #include "sysemu/sysemu.h" +#include "qapi/qmp/qdict.h" +#include "qapi/qmp/qint.h" +#include "qapi/qmp/qstring.h" #include <nfsc/libnfs.h> + #define QEMU_NFS_MAX_READAHEAD_SIZE 1048576 #define QEMU_NFS_MAX_PAGECACHE_SIZE (8388608 / NFS_BLKSIZE) #define QEMU_NFS_MAX_DEBUG_LEVEL 2 @@ -61,6 +65,118 @@ typedef struct NFSRPC { NFSClient *client; } NFSRPC; +static int nfs_parse_uri(const char *filename, QDict *options, Error **errp) +{ + URI *uri = NULL; + QueryParams *qp = NULL; + int ret = 0, i; + + uri = uri_parse(filename); + if (!uri) { + error_setg(errp, "Invalid URI specified"); + ret = -EINVAL; + goto out; + } + if (strcmp(uri->scheme, "nfs") != 0) { + error_setg(errp, "URI scheme must be 'nfs'"); + ret = -EINVAL; + goto out; + } + + if (!uri->server) { + error_setg(errp, "missing hostname in URI"); + ret = -EINVAL; + goto out; + } + + if (!uri->path) { + error_setg(errp, "missing file path in URI"); + ret = -EINVAL; + goto out; + } + + qp = query_params_parse(uri->query); + if (!qp) { + error_setg(errp, "could not parse query parameters"); + ret = -EINVAL; + goto out; + } + + qdict_put(options, "host", qstring_from_str(uri->server)); + + qdict_put(options, "path", qstring_from_str(uri->path)); + + 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; + } + + 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 +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; @@ -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())); + } + + 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 - 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"); + 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,
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 <ashijeetacharya@gmail.com> --- block/nfs.c | 348 +++++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 253 insertions(+), 95 deletions(-)