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 <ashijeetacha...@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