[Qemu-devel] [PATCH v2 0/2] block: allow blockdev-add for NFS

[Qemu-devel] [PATCH v2 0/2] block: allow blockdev-add for NFS

[Ashijeet Acharya]

Previously posted series patches:
v1: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04487.html

This series adds blockdev-add support for NFS block driver.

Patch 1 helps to prepare NFS driver to make use of several runtime_opts
as they appear in the URI. This will make NFS to do things similar to
the way other drivers available in the block layer do.

Patch 2 helps to allow blockdev-add support for the NFS block driver
by making the NFS option available.

Changes in v2:
- drop strcmp() condition check for host and path in nfs_parse_uri()
- drop "export" completely
- initialize client->context bedore setting query parameters
- fix the QDict options being passed to nfs_client_open() and make use of url

Ashijeet Acharya (2):
  block/nfs: Introduce runtime_opts in NFS
  qapi: allow blockdev-add for NFS

 block/nfs.c          | 348 +++++++++++++++++++++++++++++++++++++--------------
 qapi/block-core.json |  56 ++++++++-
 2 files changed, 305 insertions(+), 99 deletions(-)

-- 
2.6.2

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

[Ashijeet Acharya]

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));
+
+    for (i = 0; i < qp->n; i++) {
+        if (!qp->p[i].value) {
+            error_setg(errp, "Value for NFS parameter expected: %s",
+                       qp->p[i].name);
+            goto out;
+        }
+        if (parse_uint_full(qp->p[i].value, NULL, 0)) {
+            error_setg(errp, "Illegal value for NFS parameter: %s",
+                       qp->p[i].name);
+            goto out;
+        }
+        if (!strcmp(qp->p[i].name, "uid")) {
+            qdict_put(options, "uid",
+                      qstring_from_str(qp->p[i].value));
+        } else if (!strcmp(qp->p[i].name, "gid")) {
+            qdict_put(options, "gid",
+                      qstring_from_str(qp->p[i].value));
+        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
+            qdict_put(options, "tcp-syncnt",
+                      qstring_from_str(qp->p[i].value));
+        } else if (!strcmp(qp->p[i].name, "readahead")) {
+            qdict_put(options, "readahead",
+                      qstring_from_str(qp->p[i].value));
+        } else if (!strcmp(qp->p[i].name, "pagecache")) {
+            qdict_put(options, "pagecache",
+                      qstring_from_str(qp->p[i].value));
+        } else if (!strcmp(qp->p[i].name, "debug")) {
+            qdict_put(options, "debug",
+                      qstring_from_str(qp->p[i].value));
+        } else {
+            error_setg(errp, "Unknown NFS parameter name: %s",
+                       qp->p[i].name);
+            goto out;
+        }
+    }
+out:
+    if (qp) {
+        query_params_free(qp);
+    }
+    if (uri) {
+        uri_free(uri);
+    }
+    return ret;
+}
+
+static void nfs_parse_filename(const char *filename, QDict *options,
+                               Error **errp)
+{
+    int ret = 0;
+
+    if (qdict_haskey(options, "host") ||
+        qdict_haskey(options, "path") ||
+        qdict_haskey(options, "uid") ||
+        qdict_haskey(options, "gid") ||
+        qdict_haskey(options, "tcp-syncnt") ||
+        qdict_haskey(options, "readahead") ||
+        qdict_haskey(options, "pagecache") ||
+        qdict_haskey(options, "debug")) {
+        error_setg(errp, "host/path/uid/gid/tcp-syncnt/readahead/pagecache"
+                         "/debug and a filename may not be used at the same"
+                         " time");
+        return;
+    }
+
+    ret = nfs_parse_uri(filename, options, errp);
+    if (ret < 0) {
+        error_setg(errp, "No valid URL specified");
+    }
+    return;
+}
+
 static void nfs_process_read(void *arg);
 static void nfs_process_write(void *arg);
 
@@ -228,15 +344,49 @@ static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
     return task.ret;
 }
 
-/* TODO Convert to fine grained options */
 static QemuOptsList runtime_opts = {
     .name = "nfs",
     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
     .desc = {
         {
-            .name = "filename",
+            .name = "host",
+            .type = QEMU_OPT_STRING,
+            .help = "Host to connect to",
+        },
+        {
+            .name = "path",
             .type = QEMU_OPT_STRING,
-            .help = "URL to the NFS file",
+            .help = "Path of the image on the host",
+        },
+        {
+            .name = "uid",
+            .type = QEMU_OPT_NUMBER,
+            .help = "UID value to use when talking to the server",
+        },
+        {
+            .name = "gid",
+            .type = QEMU_OPT_NUMBER,
+            .help = "GID value to use when talking to the server",
+        },
+        {
+            .name = "tcp-syncnt",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Number of SYNs to send during the session establish",
+        },
+        {
+            .name = "readahead",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Set the readahead size in bytes",
+        },
+        {
+            .name = "pagecache",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Set the pagecache size in bytes",
+        },
+        {
+            .name = "debug",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Set the NFS debug level (max 2)",
         },
         { /* end of list */ }
     },
@@ -279,25 +429,40 @@ static void nfs_file_close(BlockDriverState *bs)
     nfs_client_close(client);
 }
 
-static int64_t nfs_client_open(NFSClient *client, const char *filename,
+static int64_t nfs_client_open(NFSClient *client, QDict *options,
                                int flags, Error **errp, int open_flags)
 {
-    int ret = -EINVAL, i;
+    int ret = -EINVAL;
+    QemuOpts *opts = NULL;
+    Error *local_err = NULL;
     struct stat st;
-    URI *uri;
-    QueryParams *qp = NULL;
     char *file = NULL, *strp = NULL;
+    const char *host, *path;
+    unsigned long long val;
 
-    uri = uri_parse(filename);
-    if (!uri) {
-        error_setg(errp, "Invalid URL specified");
-        goto fail;
+    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto out;
     }
-    if (!uri->server) {
-        error_setg(errp, "Invalid URL specified");
-        goto fail;
+
+    host = qemu_opt_get(opts, "host");
+    if (!host) {
+        ret = -EINVAL;
+        error_setg(errp, "No hostname was specified");
+        goto out;
     }
-    strp = strrchr(uri->path, '/');
+
+    path = qemu_opt_get(opts, "path");
+    if (!path) {
+        ret = -EINVAL;
+        error_setg(errp, "No path was specified");
+        goto out;
+    }
+
+    strp = strrchr(path, '/');
     if (strp == NULL) {
         error_setg(errp, "Invalid URL specified");
         goto fail;
@@ -311,79 +476,77 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
         goto fail;
     }
 
-    qp = query_params_parse(uri->query);
-    for (i = 0; i < qp->n; i++) {
-        unsigned long long val;
-        if (!qp->p[i].value) {
-            error_setg(errp, "Value for NFS parameter expected: %s",
-                       qp->p[i].name);
+    if (qemu_opt_get(opts, "uid")) {
+        nfs_set_uid(client->context,
+                    qemu_opt_get_number(opts, "uid", getuid()));
+    }
+
+    if (qemu_opt_get(opts, "gid")) {
+        nfs_set_gid(client->context,
+                    qemu_opt_get_number(opts, "gid", getgid()));
+    }
+
+    if (qemu_opt_get(opts, "tcp-syncnt")) {
+        nfs_set_tcp_syncnt(client->context,
+                           qemu_opt_get_number(opts, "tcp-syncnt", 0));
+    }
+
+#ifdef LIBNFS_FEATURE_READAHEAD
+    if (qemu_opt_get(opts, "readahead")) {
+        if (open_flags & BDRV_O_NOCACHE) {
+            error_setg(errp, "Cannot enable NFS readahead "
+                             "if cache.direct = on");
             goto fail;
         }
-        if (parse_uint_full(qp->p[i].value, &val, 0)) {
-            error_setg(errp, "Illegal value for NFS parameter: %s",
-                       qp->p[i].name);
-            goto fail;
+        val = qemu_opt_get_number(opts, "readahead", 0);
+        if (val > QEMU_NFS_MAX_READAHEAD_SIZE) {
+            error_report("NFS Warning: Truncating NFS readahead "
+                         "size to %d", QEMU_NFS_MAX_READAHEAD_SIZE);
+            val = QEMU_NFS_MAX_READAHEAD_SIZE;
         }
-        if (!strcmp(qp->p[i].name, "uid")) {
-            nfs_set_uid(client->context, val);
-        } else if (!strcmp(qp->p[i].name, "gid")) {
-            nfs_set_gid(client->context, val);
-        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
-            nfs_set_tcp_syncnt(client->context, val);
-#ifdef LIBNFS_FEATURE_READAHEAD
-        } else if (!strcmp(qp->p[i].name, "readahead")) {
-            if (open_flags & BDRV_O_NOCACHE) {
-                error_setg(errp, "Cannot enable NFS readahead "
-                                 "if cache.direct = on");
-                goto fail;
-            }
-            if (val > QEMU_NFS_MAX_READAHEAD_SIZE) {
-                error_report("NFS Warning: Truncating NFS readahead"
-                             " size to %d", QEMU_NFS_MAX_READAHEAD_SIZE);
-                val = QEMU_NFS_MAX_READAHEAD_SIZE;
-            }
-            nfs_set_readahead(client->context, val);
+        nfs_set_readahead(client->context, val);
 #ifdef LIBNFS_FEATURE_PAGECACHE
-            nfs_set_pagecache_ttl(client->context, 0);
+        nfs_set_pagecache_ttl(client->context, 0);
 #endif
-            client->cache_used = true;
+        client->cache_used = true;
+    }
 #endif
+
 #ifdef LIBNFS_FEATURE_PAGECACHE
-            nfs_set_pagecache_ttl(client->context, 0);
-        } else if (!strcmp(qp->p[i].name, "pagecache")) {
-            if (open_flags & BDRV_O_NOCACHE) {
-                error_setg(errp, "Cannot enable NFS pagecache "
-                                 "if cache.direct = on");
-                goto fail;
-            }
-            if (val > QEMU_NFS_MAX_PAGECACHE_SIZE) {
-                error_report("NFS Warning: Truncating NFS pagecache"
-                             " size to %d pages", QEMU_NFS_MAX_PAGECACHE_SIZE);
-                val = QEMU_NFS_MAX_PAGECACHE_SIZE;
-            }
-            nfs_set_pagecache(client->context, val);
-            nfs_set_pagecache_ttl(client->context, 0);
-            client->cache_used = true;
+    nfs_set_pagecache_ttl(client->context, 0);
+    if (qemu_opt_get(opts, "pagecache")) {
+        if (open_flags & BDRV_O_NOCACHE) {
+            error_setg(errp, "Cannot enable NFS pagecache "
+                             "if cache.direct = on");
+            goto fail;
+        }
+        val = qemu_opt_get_number(opts, "pagecache", 0);
+        if (val > QEMU_NFS_MAX_PAGECACHE_SIZE) {
+            error_report("NFS Warning: Truncating NFS pagecache "
+                         "size to %d pages", QEMU_NFS_MAX_PAGECACHE_SIZE);
+            val = QEMU_NFS_MAX_PAGECACHE_SIZE;
+        }
+        nfs_set_pagecache(client->context, val);
+        nfs_set_pagecache_ttl(client->context, 0);
+        client->cache_used = true;
+    }
 #endif
+
 #ifdef LIBNFS_FEATURE_DEBUG
-        } else if (!strcmp(qp->p[i].name, "debug")) {
-            /* limit the maximum debug level to avoid potential flooding
-             * of our log files. */
-            if (val > QEMU_NFS_MAX_DEBUG_LEVEL) {
-                error_report("NFS Warning: Limiting NFS debug level"
-                             " to %d", QEMU_NFS_MAX_DEBUG_LEVEL);
-                val = QEMU_NFS_MAX_DEBUG_LEVEL;
-            }
-            nfs_set_debug(client->context, val);
-#endif
-        } else {
-            error_setg(errp, "Unknown NFS parameter name: %s",
-                       qp->p[i].name);
-            goto fail;
+    if (qemu_opt_get(opts, "debug")) {
+        val = qemu_opt_get_number(opts, "debug", 0);
+        /* limit the maximum debug level to avoid potential flooding
+         * of our log files. */
+        if (val > QEMU_NFS_MAX_DEBUG_LEVEL) {
+            error_report("NFS Warning: Limiting NFS debug level "
+                         "to %d", QEMU_NFS_MAX_DEBUG_LEVEL);
+            val = QEMU_NFS_MAX_DEBUG_LEVEL;
         }
+        nfs_set_debug(client->context, val);
     }
+#endif
 
-    ret = nfs_mount(client->context, uri->server, uri->path);
+    ret = nfs_mount(client->context, host, path);
     if (ret < 0) {
         error_setg(errp, "Failed to mount nfs share: %s",
                    nfs_get_error(client->context));
@@ -417,13 +580,11 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
     client->st_blocks = st.st_blocks;
     client->has_zero_init = S_ISREG(st.st_mode);
     goto out;
+
 fail:
     nfs_client_close(client);
 out:
-    if (qp) {
-        query_params_free(qp);
-    }
-    uri_free(uri);
+    qemu_opts_del(opts);
     g_free(file);
     return ret;
 }
@@ -432,28 +593,17 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp) {
     NFSClient *client = bs->opaque;
     int64_t ret;
-    QemuOpts *opts;
-    Error *local_err = NULL;
 
     client->aio_context = bdrv_get_aio_context(bs);
 
-    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
-    qemu_opts_absorb_qdict(opts, options, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
-        goto out;
-    }
-    ret = nfs_client_open(client, qemu_opt_get(opts, "filename"),
+    ret = nfs_client_open(client, options,
                           (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
                           errp, bs->open_flags);
     if (ret < 0) {
-        goto out;
+        return ret;
     }
     bs->total_sectors = ret;
     ret = 0;
-out:
-    qemu_opts_del(opts);
     return ret;
 }
 
@@ -475,6 +625,7 @@ static int nfs_file_create(const char *url, QemuOpts *opts, Error **errp)
     int ret = 0;
     int64_t total_size = 0;
     NFSClient *client = g_new0(NFSClient, 1);
+    QDict *options = NULL;
 
     client->aio_context = qemu_get_aio_context();
 
@@ -482,7 +633,14 @@ static int nfs_file_create(const char *url, QemuOpts *opts, Error **errp)
     total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
                           BDRV_SECTOR_SIZE);
 
-    ret = nfs_client_open(client, url, O_CREAT, errp, 0);
+    options = qdict_new();
+    ret = nfs_parse_uri(url, options, errp);
+    if (ret < 0) {
+        error_setg(errp, "No valid URL specified");
+        goto out;
+    }
+
+    ret = nfs_client_open(client, options, O_CREAT, errp, 0);
     if (ret < 0) {
         goto out;
     }
@@ -578,7 +736,7 @@ static BlockDriver bdrv_nfs = {
     .protocol_name                  = "nfs",
 
     .instance_size                  = sizeof(NFSClient),
-    .bdrv_needs_filename            = true,
+    .bdrv_parse_filename            = nfs_parse_filename,
     .create_opts                    = &nfs_create_opts,
 
     .bdrv_has_zero_init             = nfs_has_zero_init,
-- 
2.6.2

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

[Kevin Wolf]

Peter, there is a question for you hidden somewhere below.

Am 24.10.2016 um 21:27 hat Ashijeet Acharya geschrieben:
> Make NFS block driver use various fine grained runtime_opts.
> Set .bdrv_parse_filename() to nfs_parse_filename() and introduce two
> new functions nfs_parse_filename() and nfs_parse_uri() to help parsing
> the URI.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
>  block/nfs.c | 348 +++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 253 insertions(+), 95 deletions(-)
> 
> diff --git a/block/nfs.c b/block/nfs.c
> index 8602a44..666ccf2 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -35,8 +35,12 @@
>  #include "qemu/uri.h"
>  #include "qemu/cutils.h"
>  #include "sysemu/sysemu.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qint.h"
> +#include "qapi/qmp/qstring.h"
>  #include <nfsc/libnfs.h>
>  
> +
>  #define QEMU_NFS_MAX_READAHEAD_SIZE 1048576
>  #define QEMU_NFS_MAX_PAGECACHE_SIZE (8388608 / NFS_BLKSIZE)
>  #define QEMU_NFS_MAX_DEBUG_LEVEL 2
> @@ -61,6 +65,118 @@ typedef struct NFSRPC {
>      NFSClient *client;
>  } NFSRPC;
>  
> +static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
> +{
> +    URI *uri = NULL;
> +    QueryParams *qp = NULL;
> +    int ret = 0, i;
> +
> +    uri = uri_parse(filename);
> +    if (!uri) {
> +        error_setg(errp, "Invalid URI specified");
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +    if (strcmp(uri->scheme, "nfs") != 0) {
> +        error_setg(errp, "URI scheme must be 'nfs'");
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    if (!uri->server) {
> +        error_setg(errp, "missing hostname in URI");
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    if (!uri->path) {
> +        error_setg(errp, "missing file path in URI");
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    qp = query_params_parse(uri->query);
> +    if (!qp) {
> +        error_setg(errp, "could not parse query parameters");
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    qdict_put(options, "host", qstring_from_str(uri->server));
> +
> +    qdict_put(options, "path", qstring_from_str(uri->path));

Just style, but why the empty line between both qdict_put() calls?

> +
> +    for (i = 0; i < qp->n; i++) {
> +        if (!qp->p[i].value) {
> +            error_setg(errp, "Value for NFS parameter expected: %s",
> +                       qp->p[i].name);
> +            goto out;

You need to set ret = -EINVAL here.

> +        }
> +        if (parse_uint_full(qp->p[i].value, NULL, 0)) {
> +            error_setg(errp, "Illegal value for NFS parameter: %s",
> +                       qp->p[i].name);
> +            goto out;

And here.

> +        }
> +        if (!strcmp(qp->p[i].name, "uid")) {
> +            qdict_put(options, "uid",
> +                      qstring_from_str(qp->p[i].value));
> +        } else if (!strcmp(qp->p[i].name, "gid")) {
> +            qdict_put(options, "gid",
> +                      qstring_from_str(qp->p[i].value));
> +        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
> +            qdict_put(options, "tcp-syncnt",
> +                      qstring_from_str(qp->p[i].value));
> +        } else if (!strcmp(qp->p[i].name, "readahead")) {
> +            qdict_put(options, "readahead",
> +                      qstring_from_str(qp->p[i].value));
> +        } else if (!strcmp(qp->p[i].name, "pagecache")) {
> +            qdict_put(options, "pagecache",
> +                      qstring_from_str(qp->p[i].value));
> +        } else if (!strcmp(qp->p[i].name, "debug")) {
> +            qdict_put(options, "debug",
> +                      qstring_from_str(qp->p[i].value));
> +        } else {
> +            error_setg(errp, "Unknown NFS parameter name: %s",
> +                       qp->p[i].name);
> +            goto out;

And here.

If you prefer, you can set ret = -EINVAL at the top of the function and
do a ret = 0 only right before out:

> +        }
> +    }
> +out:
> +    if (qp) {
> +        query_params_free(qp);
> +    }
> +    if (uri) {
> +        uri_free(uri);
> +    }
> +    return ret;
> +}
> +
> +static void nfs_parse_filename(const char *filename, QDict *options,
> +                               Error **errp)
> +{
> +    int ret = 0;
> +
> +    if (qdict_haskey(options, "host") ||
> +        qdict_haskey(options, "path") ||
> +        qdict_haskey(options, "uid") ||
> +        qdict_haskey(options, "gid") ||
> +        qdict_haskey(options, "tcp-syncnt") ||
> +        qdict_haskey(options, "readahead") ||
> +        qdict_haskey(options, "pagecache") ||
> +        qdict_haskey(options, "debug")) {
> +        error_setg(errp, "host/path/uid/gid/tcp-syncnt/readahead/pagecache"
> +                         "/debug and a filename may not be used at the same"
> +                         " time");
> +        return;
> +    }
> +
> +    ret = nfs_parse_uri(filename, options, errp);
> +    if (ret < 0) {
> +        error_setg(errp, "No valid URL specified");

errp has already been set by nfs_parse_uri(), you must not set it a
second time. Otherwise, this is what you get:

$ x86_64-softmmu/qemu-system-x86_64 -drive file=nfs://foo
qemu-system-x86_64: util/error.c:57: error_setv: Assertion `*errp == ((void *)0)' failed.
Aborted

> +    }
> +    return;
> +}
> +
>  static void nfs_process_read(void *arg);
>  static void nfs_process_write(void *arg);
>  
> @@ -228,15 +344,49 @@ static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
>      return task.ret;
>  }
>  
> -/* TODO Convert to fine grained options */
>  static QemuOptsList runtime_opts = {
>      .name = "nfs",
>      .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>      .desc = {
>          {
> -            .name = "filename",
> +            .name = "host",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Host to connect to",
> +        },
> +        {
> +            .name = "path",
>              .type = QEMU_OPT_STRING,
> -            .help = "URL to the NFS file",
> +            .help = "Path of the image on the host",
> +        },
> +        {
> +            .name = "uid",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "UID value to use when talking to the server",
> +        },
> +        {
> +            .name = "gid",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "GID value to use when talking to the server",
> +        },
> +        {
> +            .name = "tcp-syncnt",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "Number of SYNs to send during the session establish",
> +        },
> +        {
> +            .name = "readahead",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "Set the readahead size in bytes",
> +        },
> +        {
> +            .name = "pagecache",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "Set the pagecache size in bytes",
> +        },
> +        {
> +            .name = "debug",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "Set the NFS debug level (max 2)",
>          },
>          { /* end of list */ }
>      },
> @@ -279,25 +429,40 @@ static void nfs_file_close(BlockDriverState *bs)
>      nfs_client_close(client);
>  }
>  
> -static int64_t nfs_client_open(NFSClient *client, const char *filename,
> +static int64_t nfs_client_open(NFSClient *client, QDict *options,
>                                 int flags, Error **errp, int open_flags)
>  {
> -    int ret = -EINVAL, i;
> +    int ret = -EINVAL;
> +    QemuOpts *opts = NULL;
> +    Error *local_err = NULL;
>      struct stat st;
> -    URI *uri;
> -    QueryParams *qp = NULL;
>      char *file = NULL, *strp = NULL;
> +    const char *host, *path;
> +    unsigned long long val;
>  
> -    uri = uri_parse(filename);
> -    if (!uri) {
> -        error_setg(errp, "Invalid URL specified");
> -        goto fail;
> +    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> +    qemu_opts_absorb_qdict(opts, options, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        ret = -EINVAL;
> +        goto out;
>      }
> -    if (!uri->server) {
> -        error_setg(errp, "Invalid URL specified");
> -        goto fail;
> +
> +    host = qemu_opt_get(opts, "host");
> +    if (!host) {
> +        ret = -EINVAL;
> +        error_setg(errp, "No hostname was specified");
> +        goto out;
>      }
> -    strp = strrchr(uri->path, '/');
> +
> +    path = qemu_opt_get(opts, "path");
> +    if (!path) {
> +        ret = -EINVAL;
> +        error_setg(errp, "No path was specified");
> +        goto out;
> +    }
> +
> +    strp = strrchr(path, '/');
>      if (strp == NULL) {
>          error_setg(errp, "Invalid URL specified");
>          goto fail;

This is inconsistent. Either all error paths before initialising the
context should 'goto fail' anyway (and I'd prefer this), or they should
all 'goto out'. I don't like mixing both.

> @@ -311,79 +476,77 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
>          goto fail;
>      }
>  
> -    qp = query_params_parse(uri->query);
> -    for (i = 0; i < qp->n; i++) {
> -        unsigned long long val;
> -        if (!qp->p[i].value) {
> -            error_setg(errp, "Value for NFS parameter expected: %s",
> -                       qp->p[i].name);
> +    if (qemu_opt_get(opts, "uid")) {
> +        nfs_set_uid(client->context,
> +                    qemu_opt_get_number(opts, "uid", getuid()));

Isn't the getuid() useless here? We already check that an "uid" option
is passed (and I think doing that is correct), so the default is never
used. We could just as well pass 0 as the default.

> +    }
> +
> +    if (qemu_opt_get(opts, "gid")) {
> +        nfs_set_gid(client->context,
> +                    qemu_opt_get_number(opts, "gid", getgid()));

Here, too.

> +    }
> +
> +    if (qemu_opt_get(opts, "tcp-syncnt")) {
> +        nfs_set_tcp_syncnt(client->context,
> +                           qemu_opt_get_number(opts, "tcp-syncnt", 0));
> +    }
> +
> +#ifdef LIBNFS_FEATURE_READAHEAD
> +    if (qemu_opt_get(opts, "readahead")) {
> +        if (open_flags & BDRV_O_NOCACHE) {
> +            error_setg(errp, "Cannot enable NFS readahead "
> +                             "if cache.direct = on");
>              goto fail;
>          }
> -        if (parse_uint_full(qp->p[i].value, &val, 0)) {
> -            error_setg(errp, "Illegal value for NFS parameter: %s",
> -                       qp->p[i].name);
> -            goto fail;
> +        val = qemu_opt_get_number(opts, "readahead", 0);
> +        if (val > QEMU_NFS_MAX_READAHEAD_SIZE) {
> +            error_report("NFS Warning: Truncating NFS readahead "
> +                         "size to %d", QEMU_NFS_MAX_READAHEAD_SIZE);
> +            val = QEMU_NFS_MAX_READAHEAD_SIZE;
>          }
> -        if (!strcmp(qp->p[i].name, "uid")) {
> -            nfs_set_uid(client->context, val);
> -        } else if (!strcmp(qp->p[i].name, "gid")) {
> -            nfs_set_gid(client->context, val);
> -        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
> -            nfs_set_tcp_syncnt(client->context, val);
> -#ifdef LIBNFS_FEATURE_READAHEAD
> -        } else if (!strcmp(qp->p[i].name, "readahead")) {
> -            if (open_flags & BDRV_O_NOCACHE) {
> -                error_setg(errp, "Cannot enable NFS readahead "
> -                                 "if cache.direct = on");
> -                goto fail;
> -            }
> -            if (val > QEMU_NFS_MAX_READAHEAD_SIZE) {
> -                error_report("NFS Warning: Truncating NFS readahead"
> -                             " size to %d", QEMU_NFS_MAX_READAHEAD_SIZE);
> -                val = QEMU_NFS_MAX_READAHEAD_SIZE;
> -            }
> -            nfs_set_readahead(client->context, val);
> +        nfs_set_readahead(client->context, val);
>  #ifdef LIBNFS_FEATURE_PAGECACHE
> -            nfs_set_pagecache_ttl(client->context, 0);
> +        nfs_set_pagecache_ttl(client->context, 0);
>  #endif
> -            client->cache_used = true;
> +        client->cache_used = true;
> +    }
>  #endif
> +
>  #ifdef LIBNFS_FEATURE_PAGECACHE
> -            nfs_set_pagecache_ttl(client->context, 0);
> -        } else if (!strcmp(qp->p[i].name, "pagecache")) {
> -            if (open_flags & BDRV_O_NOCACHE) {
> -                error_setg(errp, "Cannot enable NFS pagecache "
> -                                 "if cache.direct = on");
> -                goto fail;
> -            }
> -            if (val > QEMU_NFS_MAX_PAGECACHE_SIZE) {
> -                error_report("NFS Warning: Truncating NFS pagecache"
> -                             " size to %d pages", QEMU_NFS_MAX_PAGECACHE_SIZE);
> -                val = QEMU_NFS_MAX_PAGECACHE_SIZE;
> -            }
> -            nfs_set_pagecache(client->context, val);
> -            nfs_set_pagecache_ttl(client->context, 0);
> -            client->cache_used = true;
> +    nfs_set_pagecache_ttl(client->context, 0);

We have a functional change here. I'm not completely sure yet whether
you're fixing a bug here or you're just turning one bug into a different
bug, but the old code doesn't look right anyway.

If LIBNFS_FEATURE_READAHEAD was defined, but LIBNFS_FEATURE_PAGECACHE
wasn't, the nfs_set_pagecache_ttl() would end up in the "tcp-syncnt"
option handling, which is probably completely wrong.

I suspect that commit d99b26c4 added the second call accidentally
(intended just a copy and paste of the #ifdef line?) and this line
should simply be dropped.

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

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

[Peter Lieven]

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

That line is accidently duplicated, but it has no negative impact since
if LIBNFS_FEATURE_PAGECACHE is defined, LIBNFS_FEATURE_READAHEAD
must be as well.

Peter

[Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS

[Ashijeet Acharya]

Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to
support blockdev-add for NFS network protocol driver. Also make a new
struct NFSServer to support tcp connection.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9d797b8..3ab028d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1714,9 +1714,9 @@
 { 'enum': 'BlockdevDriver',
   'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
             'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
-            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
-            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
-	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
+            'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio',
+            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
+            'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
 
 ##
 # @BlockdevOptionsFile
@@ -2212,6 +2212,54 @@
             '*top-id': 'str' } }
 
 ##
+# @NFSServer
+#
+# Captures the address of the socket
+#
+# @type:        transport type used for NFS (only TCP supported)
+#
+# @host:        host part of the address
+#
+# Since 2.8
+##
+{ 'struct': 'NFSServer',
+  'data': { 'type': 'str',
+            'host': 'str' } }
+
+##
+# @BlockdevOptionsNfs
+#
+# Driver specific block device option for NFS
+#
+# @server:        host address
+#
+# @path:          path of the image on the host
+#
+# @uid:           #optional UID value to use when talking to the server
+#
+# @gid:           #optional GID value to use when talking to the server
+#
+# @tcp-syncnt:    #optional number of SYNs during the session establishment
+#
+# @readahead:     #optional set the readahead size in bytes
+#
+# @pagecache:     #optional set the pagecache size in bytes
+#
+# @debug:         #optional set the NFS debug level (max 2)
+#
+# Since 2.8
+##
+{ 'struct': 'BlockdevOptionsNfs',
+  'data': { 'server': 'NFSServer',
+            'path': 'str',
+            '*uid': 'int',
+            '*gid': 'int',
+            '*tcp-syncnt': 'int',
+            '*readahead': 'int',
+            '*pagecache': 'int',
+            '*debug': 'int' } }
+
+##
 # @BlockdevOptionsCurl
 #
 # Driver specific block device options for the curl backend.
@@ -2269,7 +2317,7 @@
 # TODO iscsi: Wait for structured options
       'luks':       'BlockdevOptionsLUKS',
 # TODO nbd: Should take InetSocketAddress for 'host'?
-# TODO nfs: Wait for structured options
+      'nfs':        'BlockdevOptionsNfs',
       'null-aio':   'BlockdevOptionsNull',
       'null-co':    'BlockdevOptionsNull',
       'parallels':  'BlockdevOptionsGenericFormat',
-- 
2.6.2

Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS

[Kevin Wolf]

Am 24.10.2016 um 21:27 hat Ashijeet Acharya geschrieben:
> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to
> support blockdev-add for NFS network protocol driver. Also make a new
> struct NFSServer to support tcp connection.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
>  qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9d797b8..3ab028d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1714,9 +1714,9 @@
>  { 'enum': 'BlockdevDriver',
>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> -            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
> -            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> -	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> +            'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio',
> +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> +            'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
>  
>  ##
>  # @BlockdevOptionsFile
> @@ -2212,6 +2212,54 @@
>              '*top-id': 'str' } }
>  
>  ##
> +# @NFSServer
> +#
> +# Captures the address of the socket
> +#
> +# @type:        transport type used for NFS (only TCP supported)
> +#
> +# @host:        host part of the address
> +#
> +# Since 2.8
> +##
> +{ 'struct': 'NFSServer',
> +  'data': { 'type': 'str',
> +            'host': 'str' } }
> +
> +##
> +# @BlockdevOptionsNfs
> +#
> +# Driver specific block device option for NFS
> +#
> +# @server:        host address
> +#
> +# @path:          path of the image on the host
> +#
> +# @uid:           #optional UID value to use when talking to the server
> +#
> +# @gid:           #optional GID value to use when talking to the server
> +#
> +# @tcp-syncnt:    #optional number of SYNs during the session establishment
> +#
> +# @readahead:     #optional set the readahead size in bytes
> +#
> +# @pagecache:     #optional set the pagecache size in bytes
> +#
> +# @debug:         #optional set the NFS debug level (max 2)
> +#
> +# Since 2.8
> +##
> +{ 'struct': 'BlockdevOptionsNfs',
> +  'data': { 'server': 'NFSServer',

Patch 1 doesn't handle "server.type" and "server.host", so does this
actually work?

> +            'path': 'str',
> +            '*uid': 'int',
> +            '*gid': 'int',
> +            '*tcp-syncnt': 'int',
> +            '*readahead': 'int',
> +            '*pagecache': 'int',
> +            '*debug': 'int' } }
> +
> +##
>  # @BlockdevOptionsCurl
>  #
>  # Driver specific block device options for the curl backend.

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3492 bytes --]

On 10/24/2016 02:27 PM, Ashijeet Acharya wrote:
> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to
> support blockdev-add for NFS network protocol driver. Also make a new
> struct NFSServer to support tcp connection.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
>  qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9d797b8..3ab028d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1714,9 +1714,9 @@
>  { 'enum': 'BlockdevDriver',
>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> -            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
> -            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> -	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> +            'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio',
> +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> +            'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }

Missing a comment that 'nfs' is since 2.8.

>  ##
> +# @NFSServer
> +#
> +# Captures the address of the socket
> +#
> +# @type:        transport type used for NFS (only TCP supported)
> +#
> +# @host:        host part of the address
> +#
> +# Since 2.8
> +##
> +{ 'struct': 'NFSServer',
> +  'data': { 'type': 'str',

Please make this an enum, instead of an open-coded string. It's okay if
the enum only has one value 'tcp' for now; but using an enum will make
it introspectable if we later add a second transport, unlike what we get
with an open-coded string.

Must 'type' be mandatory if it must always be 'tcp'?

> +            'host': 'str' } }
> +
> +##
> +# @BlockdevOptionsNfs
> +#
> +# Driver specific block device option for NFS
> +#
> +# @server:        host address
> +#
> +# @path:          path of the image on the host
> +#
> +# @uid:           #optional UID value to use when talking to the server
> +#
> +# @gid:           #optional GID value to use when talking to the server

Do we want to allow string names in addition to numeric uid/gid values?
I'm not sure if NFS has name-based id mapping, but it's food for thought
on whether we need to use an alternate type here (alternate between
integer id and string name), or leave this as is.

> +#
> +# @tcp-syncnt:    #optional number of SYNs during the session establishment

Would tcp-syn-count be any more legible?  What is the default when omitted?

> +#
> +# @readahead:     #optional set the readahead size in bytes

What's the default when omitted?

> +#
> +# @pagecache:     #optional set the pagecache size in bytes

Default?

> +#
> +# @debug:         #optional set the NFS debug level (max 2)

Presumably default 0?

> +#
> +# Since 2.8
> +##
> +{ 'struct': 'BlockdevOptionsNfs',
> +  'data': { 'server': 'NFSServer',
> +            'path': 'str',
> +            '*uid': 'int',
> +            '*gid': 'int',
> +            '*tcp-syncnt': 'int',
> +            '*readahead': 'int',
> +            '*pagecache': 'int',
> +            '*debug': 'int' } }
> +

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS

[Markus Armbruster]

A few drive-by comments...

Eric Blake <eblake@redhat.com> writes:

> On 10/24/2016 02:27 PM, Ashijeet Acharya wrote:
>> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to
>> support blockdev-add for NFS network protocol driver. Also make a new
>> struct NFSServer to support tcp connection.
>> 
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> ---
>>  qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 52 insertions(+), 4 deletions(-)
>> 
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 9d797b8..3ab028d 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1714,9 +1714,9 @@
>>  { 'enum': 'BlockdevDriver',
>>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
>>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
>> -            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
>> -            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
>> -	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
>> +            'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio',
>> +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
>> +            'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
>
> Missing a comment that 'nfs' is since 2.8.
>
>>  ##
>> +# @NFSServer
>> +#
>> +# Captures the address of the socket
>> +#
>> +# @type:        transport type used for NFS (only TCP supported)
>> +#
>> +# @host:        host part of the address
>> +#
>> +# Since 2.8
>> +##
>> +{ 'struct': 'NFSServer',
>> +  'data': { 'type': 'str',
>
> Please make this an enum, instead of an open-coded string. It's okay if
> the enum only has one value 'tcp' for now; but using an enum will make
> it introspectable if we later add a second transport, unlike what we get
> with an open-coded string.

Yes.  When a JSON string has a compile-time fixed set of values, 'str'
is generally wrong.

> Must 'type' be mandatory if it must always be 'tcp'?
>
>> +            'host': 'str' } }
>> +
>> +##
>> +# @BlockdevOptionsNfs
>> +#
>> +# Driver specific block device option for NFS
>> +#
>> +# @server:        host address
>> +#
>> +# @path:          path of the image on the host
>> +#
>> +# @uid:           #optional UID value to use when talking to the server
>> +#
>> +# @gid:           #optional GID value to use when talking to the server
>
> Do we want to allow string names in addition to numeric uid/gid values?
> I'm not sure if NFS has name-based id mapping, but it's food for thought
> on whether we need to use an alternate type here (alternate between
> integer id and string name), or leave this as is.

As far as I know, NFS4 supports user and group names.  On Linux, see
rpc.idmapd(8).

How the name support affects C code I can't say.  If it's transparent,
i.e. you simply use local UID/GID, and the mapping happens below the
hood, then we'd have to translate string values to local UID/GID by the
usual means.  Looks like a minor convenience feature on first glance.
However, QMP is a *network* protocol.  A remote client can't easily do
this translation.

Consider a GUI like virt-manager: I guess we'd rather support user and
group names there.  But if we do, and QMP doesn't, either virt-manager
or libvirt need to map to numeric IDs.  Easy enough when running on the
host, probably impractical when not.

If we permit string values, are @uid and @gid still appropriate names?
The user name is not the user ID, it just maps to it.

>> +#
>> +# @tcp-syncnt:    #optional number of SYNs during the session establishment
>
> Would tcp-syn-count be any more legible?

We generally write out things in long hand in the QAPI schema.

>                                           What is the default when omitted?

Whenever you write #optional, you must explain the default.  When
the default is a fixed value, specify it.  When the system picks a
default, state that, and think hard about what you need to specify on
how the system picks.

>> +#
>> +# @readahead:     #optional set the readahead size in bytes

@read-ahead

> What's the default when omitted?
>
>> +#
>> +# @pagecache:     #optional set the pagecache size in bytes

@page-cache

> Default?
>
>> +#
>> +# @debug:         #optional set the NFS debug level (max 2)
>
> Presumably default 0?

@BlockdevOptionsGluster calls this @debug-level.

>> +#
>> +# Since 2.8
>> +##
>> +{ 'struct': 'BlockdevOptionsNfs',
>> +  'data': { 'server': 'NFSServer',
>> +            'path': 'str',
>> +            '*uid': 'int',
>> +            '*gid': 'int',
>> +            '*tcp-syncnt': 'int',
>> +            '*readahead': 'int',
>> +            '*pagecache': 'int',
>> +            '*debug': 'int' } }
>> +

Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS

[Kevin Wolf]

Am 26.10.2016 um 09:23 hat Markus Armbruster geschrieben:
> A few drive-by comments...
> 
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 10/24/2016 02:27 PM, Ashijeet Acharya wrote:
> >> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to
> >> support blockdev-add for NFS network protocol driver. Also make a new
> >> struct NFSServer to support tcp connection.
> >> 
> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> >> ---
> >>  qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
> >>  1 file changed, 52 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> index 9d797b8..3ab028d 100644
> >> --- a/qapi/block-core.json
> >> +++ b/qapi/block-core.json
> >> @@ -1714,9 +1714,9 @@
> >>  { 'enum': 'BlockdevDriver',
> >>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
> >>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> >> -            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
> >> -            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> >> -	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> >> +            'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio',
> >> +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> >> +            'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> >
> > Missing a comment that 'nfs' is since 2.8.
> >
> >>  ##
> >> +# @NFSServer
> >> +#
> >> +# Captures the address of the socket
> >> +#
> >> +# @type:        transport type used for NFS (only TCP supported)
> >> +#
> >> +# @host:        host part of the address
> >> +#
> >> +# Since 2.8
> >> +##
> >> +{ 'struct': 'NFSServer',
> >> +  'data': { 'type': 'str',
> >
> > Please make this an enum, instead of an open-coded string. It's okay if
> > the enum only has one value 'tcp' for now; but using an enum will make
> > it introspectable if we later add a second transport, unlike what we get
> > with an open-coded string.
> 
> Yes.  When a JSON string has a compile-time fixed set of values, 'str'
> is generally wrong.
> 
> > Must 'type' be mandatory if it must always be 'tcp'?
> >
> >> +            'host': 'str' } }
> >> +
> >> +##
> >> +# @BlockdevOptionsNfs
> >> +#
> >> +# Driver specific block device option for NFS
> >> +#
> >> +# @server:        host address
> >> +#
> >> +# @path:          path of the image on the host
> >> +#
> >> +# @uid:           #optional UID value to use when talking to the server
> >> +#
> >> +# @gid:           #optional GID value to use when talking to the server
> >
> > Do we want to allow string names in addition to numeric uid/gid values?
> > I'm not sure if NFS has name-based id mapping, but it's food for thought
> > on whether we need to use an alternate type here (alternate between
> > integer id and string name), or leave this as is.
> 
> As far as I know, NFS4 supports user and group names.  On Linux, see
> rpc.idmapd(8).
> 
> How the name support affects C code I can't say.  If it's transparent,
> i.e. you simply use local UID/GID, and the mapping happens below the
> hood, then we'd have to translate string values to local UID/GID by the
> usual means.  Looks like a minor convenience feature on first glance.
> However, QMP is a *network* protocol.  A remote client can't easily do
> this translation.
> 
> Consider a GUI like virt-manager: I guess we'd rather support user and
> group names there.  But if we do, and QMP doesn't, either virt-manager
> or libvirt need to map to numeric IDs.  Easy enough when running on the
> host, probably impractical when not.
> 
> If we permit string values, are @uid and @gid still appropriate names?
> The user name is not the user ID, it just maps to it.
>
> >> +#
> >> +# @tcp-syncnt:    #optional number of SYNs during the session establishment
> >
> > Would tcp-syn-count be any more legible?
> 
> We generally write out things in long hand in the QAPI schema.
> 
> >                                           What is the default when omitted?
> 
> Whenever you write #optional, you must explain the default.  When
> the default is a fixed value, specify it.  When the system picks a
> default, state that, and think hard about what you need to specify on
> how the system picks.
> 
> >> +#
> >> +# @readahead:     #optional set the readahead size in bytes
> 
> @read-ahead
> 
> > What's the default when omitted?
> >
> >> +#
> >> +# @pagecache:     #optional set the pagecache size in bytes
> 
> @page-cache
> 
> > Default?
> >
> >> +#
> >> +# @debug:         #optional set the NFS debug level (max 2)
> >
> > Presumably default 0?
> 
> @BlockdevOptionsGluster calls this @debug-level.

Are all of these renames really worth having to support two option
names for each option in the driver? I'm not sure if I'm convinced of
this.

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 26.10.2016 um 09:23 hat Markus Armbruster geschrieben:
>> A few drive-by comments...
>> 
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 10/24/2016 02:27 PM, Ashijeet Acharya wrote:
>> >> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to
>> >> support blockdev-add for NFS network protocol driver. Also make a new
>> >> struct NFSServer to support tcp connection.
>> >> 
>> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> >> ---
>> >>  qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
>> >>  1 file changed, 52 insertions(+), 4 deletions(-)
>> >> 
>> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> index 9d797b8..3ab028d 100644
>> >> --- a/qapi/block-core.json
>> >> +++ b/qapi/block-core.json
>> >> @@ -1714,9 +1714,9 @@
>> >>  { 'enum': 'BlockdevDriver',
>> >>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
>> >>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
>> >> -            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
>> >> -            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
>> >> -	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
>> >> +            'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio',
>> >> +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
>> >> +            'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
>> >
>> > Missing a comment that 'nfs' is since 2.8.
>> >
>> >>  ##
>> >> +# @NFSServer
>> >> +#
>> >> +# Captures the address of the socket
>> >> +#
>> >> +# @type:        transport type used for NFS (only TCP supported)
>> >> +#
>> >> +# @host:        host part of the address
>> >> +#
>> >> +# Since 2.8
>> >> +##
>> >> +{ 'struct': 'NFSServer',
>> >> +  'data': { 'type': 'str',
>> >
>> > Please make this an enum, instead of an open-coded string. It's okay if
>> > the enum only has one value 'tcp' for now; but using an enum will make
>> > it introspectable if we later add a second transport, unlike what we get
>> > with an open-coded string.
>> 
>> Yes.  When a JSON string has a compile-time fixed set of values, 'str'
>> is generally wrong.
>> 
>> > Must 'type' be mandatory if it must always be 'tcp'?
>> >
>> >> +            'host': 'str' } }
>> >> +
>> >> +##
>> >> +# @BlockdevOptionsNfs
>> >> +#
>> >> +# Driver specific block device option for NFS
>> >> +#
>> >> +# @server:        host address
>> >> +#
>> >> +# @path:          path of the image on the host
>> >> +#
>> >> +# @uid:           #optional UID value to use when talking to the server
>> >> +#
>> >> +# @gid:           #optional GID value to use when talking to the server
>> >
>> > Do we want to allow string names in addition to numeric uid/gid values?
>> > I'm not sure if NFS has name-based id mapping, but it's food for thought
>> > on whether we need to use an alternate type here (alternate between
>> > integer id and string name), or leave this as is.
>> 
>> As far as I know, NFS4 supports user and group names.  On Linux, see
>> rpc.idmapd(8).
>> 
>> How the name support affects C code I can't say.  If it's transparent,
>> i.e. you simply use local UID/GID, and the mapping happens below the
>> hood, then we'd have to translate string values to local UID/GID by the
>> usual means.  Looks like a minor convenience feature on first glance.
>> However, QMP is a *network* protocol.  A remote client can't easily do
>> this translation.
>> 
>> Consider a GUI like virt-manager: I guess we'd rather support user and
>> group names there.  But if we do, and QMP doesn't, either virt-manager
>> or libvirt need to map to numeric IDs.  Easy enough when running on the
>> host, probably impractical when not.
>> 
>> If we permit string values, are @uid and @gid still appropriate names?
>> The user name is not the user ID, it just maps to it.
>>
>> >> +#
>> >> +# @tcp-syncnt:    #optional number of SYNs during the session establishment
>> >
>> > Would tcp-syn-count be any more legible?
>> 
>> We generally write out things in long hand in the QAPI schema.
>> 
>> >                                           What is the default when omitted?
>> 
>> Whenever you write #optional, you must explain the default.  When
>> the default is a fixed value, specify it.  When the system picks a
>> default, state that, and think hard about what you need to specify on
>> how the system picks.
>> 
>> >> +#
>> >> +# @readahead:     #optional set the readahead size in bytes
>> 
>> @read-ahead
>> 
>> > What's the default when omitted?
>> >
>> >> +#
>> >> +# @pagecache:     #optional set the pagecache size in bytes
>> 
>> @page-cache
>> 
>> > Default?
>> >
>> >> +#
>> >> +# @debug:         #optional set the NFS debug level (max 2)
>> >
>> > Presumably default 0?
>> 
>> @BlockdevOptionsGluster calls this @debug-level.
>
> Are all of these renames really worth having to support two option
> names for each option in the driver? I'm not sure if I'm convinced of
> this.

I don't see "two option names for each option" in this patch, so please
explain.

Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS

[Kevin Wolf]

Am 26.10.2016 um 10:40 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 26.10.2016 um 09:23 hat Markus Armbruster geschrieben:
> >> A few drive-by comments...
> >> 
> >> Eric Blake <eblake@redhat.com> writes:
> >> 
> >> > On 10/24/2016 02:27 PM, Ashijeet Acharya wrote:
> >> >> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to
> >> >> support blockdev-add for NFS network protocol driver. Also make a new
> >> >> struct NFSServer to support tcp connection.
> >> >> 
> >> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> >> >> ---
> >> >>  qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
> >> >>  1 file changed, 52 insertions(+), 4 deletions(-)
> >> >> 
> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> >> index 9d797b8..3ab028d 100644
> >> >> --- a/qapi/block-core.json
> >> >> +++ b/qapi/block-core.json
> >> >> @@ -1714,9 +1714,9 @@
> >> >>  { 'enum': 'BlockdevDriver',
> >> >>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
> >> >>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> >> >> -            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
> >> >> -            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> >> >> -	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> >> >> +            'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio',
> >> >> +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> >> >> +            'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> >> >
> >> > Missing a comment that 'nfs' is since 2.8.
> >> >
> >> >>  ##
> >> >> +# @NFSServer
> >> >> +#
> >> >> +# Captures the address of the socket
> >> >> +#
> >> >> +# @type:        transport type used for NFS (only TCP supported)
> >> >> +#
> >> >> +# @host:        host part of the address
> >> >> +#
> >> >> +# Since 2.8
> >> >> +##
> >> >> +{ 'struct': 'NFSServer',
> >> >> +  'data': { 'type': 'str',
> >> >
> >> > Please make this an enum, instead of an open-coded string. It's okay if
> >> > the enum only has one value 'tcp' for now; but using an enum will make
> >> > it introspectable if we later add a second transport, unlike what we get
> >> > with an open-coded string.
> >> 
> >> Yes.  When a JSON string has a compile-time fixed set of values, 'str'
> >> is generally wrong.
> >> 
> >> > Must 'type' be mandatory if it must always be 'tcp'?
> >> >
> >> >> +            'host': 'str' } }
> >> >> +
> >> >> +##
> >> >> +# @BlockdevOptionsNfs
> >> >> +#
> >> >> +# Driver specific block device option for NFS
> >> >> +#
> >> >> +# @server:        host address
> >> >> +#
> >> >> +# @path:          path of the image on the host
> >> >> +#
> >> >> +# @uid:           #optional UID value to use when talking to the server
> >> >> +#
> >> >> +# @gid:           #optional GID value to use when talking to the server
> >> >
> >> > Do we want to allow string names in addition to numeric uid/gid values?
> >> > I'm not sure if NFS has name-based id mapping, but it's food for thought
> >> > on whether we need to use an alternate type here (alternate between
> >> > integer id and string name), or leave this as is.
> >> 
> >> As far as I know, NFS4 supports user and group names.  On Linux, see
> >> rpc.idmapd(8).
> >> 
> >> How the name support affects C code I can't say.  If it's transparent,
> >> i.e. you simply use local UID/GID, and the mapping happens below the
> >> hood, then we'd have to translate string values to local UID/GID by the
> >> usual means.  Looks like a minor convenience feature on first glance.
> >> However, QMP is a *network* protocol.  A remote client can't easily do
> >> this translation.
> >> 
> >> Consider a GUI like virt-manager: I guess we'd rather support user and
> >> group names there.  But if we do, and QMP doesn't, either virt-manager
> >> or libvirt need to map to numeric IDs.  Easy enough when running on the
> >> host, probably impractical when not.
> >> 
> >> If we permit string values, are @uid and @gid still appropriate names?
> >> The user name is not the user ID, it just maps to it.
> >>
> >> >> +#
> >> >> +# @tcp-syncnt:    #optional number of SYNs during the session establishment
> >> >
> >> > Would tcp-syn-count be any more legible?
> >> 
> >> We generally write out things in long hand in the QAPI schema.
> >> 
> >> >                                           What is the default when omitted?
> >> 
> >> Whenever you write #optional, you must explain the default.  When
> >> the default is a fixed value, specify it.  When the system picks a
> >> default, state that, and think hard about what you need to specify on
> >> how the system picks.
> >> 
> >> >> +#
> >> >> +# @readahead:     #optional set the readahead size in bytes
> >> 
> >> @read-ahead
> >> 
> >> > What's the default when omitted?
> >> >
> >> >> +#
> >> >> +# @pagecache:     #optional set the pagecache size in bytes
> >> 
> >> @page-cache
> >> 
> >> > Default?
> >> >
> >> >> +#
> >> >> +# @debug:         #optional set the NFS debug level (max 2)
> >> >
> >> > Presumably default 0?
> >> 
> >> @BlockdevOptionsGluster calls this @debug-level.
> >
> > Are all of these renames really worth having to support two option
> > names for each option in the driver? I'm not sure if I'm convinced of
> > this.
> 
> I don't see "two option names for each option" in this patch, so please
> explain.

You're right.

I was assuming that this patch just exposes existing command line
options as they are. But before patch 1 of this series, the options
aren't actual command line options, but just the query parameter names
in URIs. As we have to put these into the QDict explicitly anyway in
nfs_parse_uri(), we can still make a choice on the name of the options
there.

So I support renaming things before we expose them. I'll reply again to
your original proposal for the exact naming.

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS

[Kevin Wolf]

Am 26.10.2016 um 09:23 hat Markus Armbruster geschrieben:
> A few drive-by comments...
> 
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 10/24/2016 02:27 PM, Ashijeet Acharya wrote:
> >> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to
> >> support blockdev-add for NFS network protocol driver. Also make a new
> >> struct NFSServer to support tcp connection.
> >> 
> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> >> ---
> >>  qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
> >>  1 file changed, 52 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> index 9d797b8..3ab028d 100644
> >> --- a/qapi/block-core.json
> >> +++ b/qapi/block-core.json
> >> @@ -1714,9 +1714,9 @@
> >>  { 'enum': 'BlockdevDriver',
> >>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
> >>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> >> -            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
> >> -            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> >> -	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> >> +            'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio',
> >> +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> >> +            'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> >
> > Missing a comment that 'nfs' is since 2.8.
> >
> >>  ##
> >> +# @NFSServer
> >> +#
> >> +# Captures the address of the socket
> >> +#
> >> +# @type:        transport type used for NFS (only TCP supported)
> >> +#
> >> +# @host:        host part of the address
> >> +#
> >> +# Since 2.8
> >> +##
> >> +{ 'struct': 'NFSServer',
> >> +  'data': { 'type': 'str',
> >
> > Please make this an enum, instead of an open-coded string. It's okay if
> > the enum only has one value 'tcp' for now; but using an enum will make
> > it introspectable if we later add a second transport, unlike what we get
> > with an open-coded string.
> 
> Yes.  When a JSON string has a compile-time fixed set of values, 'str'
> is generally wrong.
> 
> > Must 'type' be mandatory if it must always be 'tcp'?
> >
> >> +            'host': 'str' } }
> >> +
> >> +##
> >> +# @BlockdevOptionsNfs
> >> +#
> >> +# Driver specific block device option for NFS
> >> +#
> >> +# @server:        host address
> >> +#
> >> +# @path:          path of the image on the host
> >> +#
> >> +# @uid:           #optional UID value to use when talking to the server
> >> +#
> >> +# @gid:           #optional GID value to use when talking to the server
> >
> > Do we want to allow string names in addition to numeric uid/gid values?
> > I'm not sure if NFS has name-based id mapping, but it's food for thought
> > on whether we need to use an alternate type here (alternate between
> > integer id and string name), or leave this as is.
> 
> As far as I know, NFS4 supports user and group names.  On Linux, see
> rpc.idmapd(8).
> 
> How the name support affects C code I can't say.  If it's transparent,
> i.e. you simply use local UID/GID, and the mapping happens below the
> hood, then we'd have to translate string values to local UID/GID by the
> usual means.  Looks like a minor convenience feature on first glance.
> However, QMP is a *network* protocol.  A remote client can't easily do
> this translation.
> 
> Consider a GUI like virt-manager: I guess we'd rather support user and
> group names there.  But if we do, and QMP doesn't, either virt-manager
> or libvirt need to map to numeric IDs.  Easy enough when running on the
> host, probably impractical when not.
> 
> If we permit string values, are @uid and @gid still appropriate names?
> The user name is not the user ID, it just maps to it.

I guess we can make it @user and @group now, and then leave it an int
for now so that making it an alternate later is introspectable?

> >> +#
> >> +# @tcp-syncnt:    #optional number of SYNs during the session establishment
> >
> > Would tcp-syn-count be any more legible?

Looks good to me.

> We generally write out things in long hand in the QAPI schema.
> 
> >                                           What is the default when omitted?
> 
> Whenever you write #optional, you must explain the default.  When
> the default is a fixed value, specify it.  When the system picks a
> default, state that, and think hard about what you need to specify on
> how the system picks.
> 
> >> +#
> >> +# @readahead:     #optional set the readahead size in bytes
> 
> @read-ahead

I think this is generally considered a single word. But @readahead-size.

> > What's the default when omitted?
> >
> >> +#
> >> +# @pagecache:     #optional set the pagecache size in bytes
> 
> @page-cache

@page-cache-size

> > Default?
> >
> >> +#
> >> +# @debug:         #optional set the NFS debug level (max 2)
> >
> > Presumably default 0?
> 
> @BlockdevOptionsGluster calls this @debug-level.

I wouldn't mind either way, but if there's precedence, let's do the same
for consistency.

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

Am 25.10.2016 um 23:16 hat Eric Blake geschrieben:
> On 10/24/2016 02:27 PM, Ashijeet Acharya wrote:
> > Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to
> > support blockdev-add for NFS network protocol driver. Also make a new
> > struct NFSServer to support tcp connection.
> > 
> > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> > ---
> >  qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 52 insertions(+), 4 deletions(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 9d797b8..3ab028d 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1714,9 +1714,9 @@
> >  { 'enum': 'BlockdevDriver',
> >    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
> >              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> > -            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
> > -            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> > -	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> > +            'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio',
> > +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> > +            'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> 
> Missing a comment that 'nfs' is since 2.8.
> 
> >  ##
> > +# @NFSServer
> > +#
> > +# Captures the address of the socket
> > +#
> > +# @type:        transport type used for NFS (only TCP supported)
> > +#
> > +# @host:        host part of the address
> > +#
> > +# Since 2.8
> > +##
> > +{ 'struct': 'NFSServer',
> > +  'data': { 'type': 'str',
> 
> Please make this an enum, instead of an open-coded string. It's okay if
> the enum only has one value 'tcp' for now; but using an enum will make
> it introspectable if we later add a second transport, unlike what we get
> with an open-coded string.
> 
> Must 'type' be mandatory if it must always be 'tcp'?

I think the idea here was to make the wire format compatible with
SocketAddress so we could later extend it. So it any case, it should be
'inet' rather than 'tcp'.

Using an enum is probably a good idea, too.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS

[Ashijeet Acharya]

On Wed, Oct 26, 2016 at 3:19 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 25.10.2016 um 23:16 hat Eric Blake geschrieben:
>> On 10/24/2016 02:27 PM, Ashijeet Acharya wrote:
>> > Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to
>> > support blockdev-add for NFS network protocol driver. Also make a new
>> > struct NFSServer to support tcp connection.
>> >
>> > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> > ---
>> >  qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++----
>> >  1 file changed, 52 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index 9d797b8..3ab028d 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -1714,9 +1714,9 @@
>> >  { 'enum': 'BlockdevDriver',
>> >    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
>> >              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
>> > -            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
>> > -            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
>> > -       'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
>> > +            'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio',
>> > +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
>> > +            'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
>>
>> Missing a comment that 'nfs' is since 2.8.
>>
>> >  ##
>> > +# @NFSServer
>> > +#
>> > +# Captures the address of the socket
>> > +#
>> > +# @type:        transport type used for NFS (only TCP supported)
>> > +#
>> > +# @host:        host part of the address
>> > +#
>> > +# Since 2.8
>> > +##
>> > +{ 'struct': 'NFSServer',
>> > +  'data': { 'type': 'str',
>>
>> Please make this an enum, instead of an open-coded string. It's okay if
>> the enum only has one value 'tcp' for now; but using an enum will make
>> it introspectable if we later add a second transport, unlike what we get
>> with an open-coded string.
>>
>> Must 'type' be mandatory if it must always be 'tcp'?
>
> I think the idea here was to make the wire format compatible with
> SocketAddress so we could later extend it. So it any case, it should be
> 'inet' rather than 'tcp'.
>
> Using an enum is probably a good idea, too.

Making it an enum and converting it to a flat union like the way
gluster does and set tcp (or inet as you prefer) to something like
NFSSocketAddress which contains only host for now? That way we will
just have to expand the enum in the future.

Ashijeet
>
> Kevin