Re: [Qemu-devel] [PATCH v2 1/2] block/nfs: Introduce runtime_opts in NFS

[Kevin Wolf <kwolf@redhat.com>]

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