On Mon, Oct 24, 2016 at 8:40 PM, Kevin Wolf <kw...@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 <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.
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