| Message ID | 1476880711-854-2-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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,
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(-)