[Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Ashijeet Acharya]

Previously posted series patches:
v5: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07580.html
v4: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07449.html
v3: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06903.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg05844.html
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. It also adds support
to handle new option "server".

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

Changes in v6:
- include only uid and gid in exact_filename
- include filename along with the path
- use visit_free()
- fix the assertion issue

Changes in v5:
- drop use of legacy options
- set bs->exact_filename in bdrv_refresh_filename
- change client->inet to client->server
- fix qdict_crumple() bug

Changes in v4:
- add support for option "server" in nfs driver
- add bdrv_refresh_filename fix
- fix the comments in json

Changes in v3:
- minor coding style fix
- set ret=-EINVAL in nfs_parse_uri()
- fix the bug of setting errp twice
- make all error paths 'goto fail'
- pass 0 as a default value in qemu_opt_get_number()
- drop nfs_set_pagecache_ttl()
- introduce new enum NFSTransport and set 'type' to use it NFSServer
- mention default values of query parameters
- change the names of query parameters in JSON

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          | 442 ++++++++++++++++++++++++++++++++++++++++-----------
 qapi/block-core.json |  74 ++++++++-
 2 files changed, 420 insertions(+), 96 deletions(-)

-- 
2.6.2

[Qemu-devel] [PATCH v6 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.
Add a new option "server" which then accepts a new struct NFSServer.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 block/nfs.c | 442 +++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 349 insertions(+), 93 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index c3db2ec..487d798 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -35,8 +35,15 @@
 #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 "qapi-visit.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.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
@@ -49,6 +56,9 @@ typedef struct NFSClient {
     AioContext *aio_context;
     blkcnt_t st_blocks;
     bool cache_used;
+    NFSServer *server;
+    char *path;
+    int64_t uid, gid, tcp_syncnt, readahead, pagecache, debug;
 } NFSClient;
 
 typedef struct NFSRPC {
@@ -60,6 +70,122 @@ 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 = -EINVAL, i;
+
+    uri = uri_parse(filename);
+    if (!uri) {
+        error_setg(errp, "Invalid URI specified");
+        goto out;
+    }
+    if (strcmp(uri->scheme, "nfs") != 0) {
+        error_setg(errp, "URI scheme must be 'nfs'");
+        goto out;
+    }
+
+    if (!uri->server) {
+        error_setg(errp, "missing hostname in URI");
+        goto out;
+    }
+
+    if (!uri->path) {
+        error_setg(errp, "missing file path in URI");
+        goto out;
+    }
+
+    qp = query_params_parse(uri->query);
+    if (!qp) {
+        error_setg(errp, "could not parse query parameters");
+        goto out;
+    }
+
+    qdict_put(options, "server.host", qstring_from_str(uri->server));
+    qdict_put(options, "server.type", qstring_from_str("inet"));
+    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, "user",
+                      qstring_from_str(qp->p[i].value));
+        } else if (!strcmp(qp->p[i].name, "gid")) {
+            qdict_put(options, "group",
+                      qstring_from_str(qp->p[i].value));
+        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
+            qdict_put(options, "tcp-syn-count",
+                      qstring_from_str(qp->p[i].value));
+        } else if (!strcmp(qp->p[i].name, "readahead")) {
+            qdict_put(options, "readahead-size",
+                      qstring_from_str(qp->p[i].value));
+        } else if (!strcmp(qp->p[i].name, "pagecache")) {
+            qdict_put(options, "page-cache-size",
+                      qstring_from_str(qp->p[i].value));
+        } else if (!strcmp(qp->p[i].name, "debug")) {
+            qdict_put(options, "debug-level",
+                      qstring_from_str(qp->p[i].value));
+        } else {
+            error_setg(errp, "Unknown NFS parameter name: %s",
+                       qp->p[i].name);
+            goto out;
+        }
+    }
+    ret = 0;
+out:
+    if (qp) {
+        query_params_free(qp);
+    }
+    if (uri) {
+        uri_free(uri);
+    }
+    return ret;
+}
+
+static bool nfs_has_filename_options_conflict(QDict *options, Error **errp)
+{
+    const QDictEntry *qe;
+
+    for (qe = qdict_first(options); qe; qe = qdict_next(options, qe)) {
+        if (!strcmp(qe->key, "host") ||
+            !strcmp(qe->key, "path") ||
+            !strcmp(qe->key, "user") ||
+            !strcmp(qe->key, "group") ||
+            !strcmp(qe->key, "tcp-syn-count") ||
+            !strcmp(qe->key, "readahead-size") ||
+            !strcmp(qe->key, "page-cache-size") ||
+            !strcmp(qe->key, "debug-level") ||
+            strstart(qe->key, "server.", NULL))
+        {
+            error_setg(errp, "Option %s cannot be used with a filename",
+                       qe->key);
+            return true;
+        }
+    }
+
+    return false;
+}
+
+static void nfs_parse_filename(const char *filename, QDict *options,
+                               Error **errp)
+{
+    if (nfs_has_filename_options_conflict(options, errp)) {
+        return;
+    }
+
+    nfs_parse_uri(filename, options, errp);
+}
+
 static void nfs_process_read(void *arg);
 static void nfs_process_write(void *arg);
 
@@ -225,15 +351,44 @@ 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 = "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 */ }
     },
@@ -276,25 +431,65 @@ static void nfs_file_close(BlockDriverState *bs)
     nfs_client_close(client);
 }
 
-static int64_t nfs_client_open(NFSClient *client, const char *filename,
+static NFSServer *nfs_config(QDict *options, Error **errp)
+{
+    NFSServer *server = NULL;
+    QDict *addr = NULL;
+    QObject *crumpled_addr = NULL;
+    Visitor *iv = NULL;
+    Error *local_error = NULL;
+
+    qdict_extract_subqdict(options, &addr, "server.");
+    if (!qdict_size(addr)) {
+        error_setg(errp, "NFS server address missing");
+        goto out;
+    }
+
+    crumpled_addr = qdict_crumple(addr, errp);
+    if (!crumpled_addr) {
+        goto out;
+    }
+
+    iv = qobject_input_visitor_new(crumpled_addr, true);
+    visit_type_NFSServer(iv, NULL, &server, &local_error);
+    if (local_error) {
+        error_propagate(errp, local_error);
+        goto out;
+    }
+
+out:
+    QDECREF(addr);
+    qobject_decref(crumpled_addr);
+    visit_free(iv);
+    return server;
+}
+
+
+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;
 
-    uri = uri_parse(filename);
-    if (!uri) {
-        error_setg(errp, "Invalid URL specified");
+    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 fail;
     }
-    if (!uri->server) {
-        error_setg(errp, "Invalid URL specified");
+
+    client->path = g_strdup(qemu_opt_get(opts, "path"));
+    if (!client->path) {
+        ret = -EINVAL;
+        error_setg(errp, "No path was specified");
         goto fail;
     }
-    strp = strrchr(uri->path, '/');
+
+    strp = strrchr(client->path, '/');
     if (strp == NULL) {
         error_setg(errp, "Invalid URL specified");
         goto fail;
@@ -302,85 +497,89 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
     file = g_strdup(strp);
     *strp = 0;
 
+    /* Pop the config into our state object, Exit if invalid */
+    client->server = nfs_config(options, errp);
+    if (!client->server) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
     client->context = nfs_init_context();
     if (client->context == NULL) {
         error_setg(errp, "Failed to init NFS context");
         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")) {
+        client->uid = qemu_opt_get_number(opts, "uid", 0);
+        nfs_set_uid(client->context, client->uid);
+    }
+
+    if (qemu_opt_get(opts, "gid")) {
+        client->gid = qemu_opt_get_number(opts, "gid", 0);
+        nfs_set_gid(client->context, client->gid);
+    }
+
+    if (qemu_opt_get(opts, "tcp-syncnt")) {
+        client->tcp_syncnt = qemu_opt_get_number(opts, "tcp-syncnt", 0);
+        nfs_set_tcp_syncnt(client->context, client->tcp_syncnt);
+    }
+
+#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;
+        client->readahead = qemu_opt_get_number(opts, "readahead", 0);
+        if (client->readahead > QEMU_NFS_MAX_READAHEAD_SIZE) {
+            error_report("NFS Warning: Truncating NFS readahead "
+                         "size to %d", QEMU_NFS_MAX_READAHEAD_SIZE);
+            client->readahead = 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, client->readahead);
 #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;
+    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;
+        }
+        client->pagecache = qemu_opt_get_number(opts, "pagecache", 0);
+        if (client->pagecache > QEMU_NFS_MAX_PAGECACHE_SIZE) {
+            error_report("NFS Warning: Truncating NFS pagecache "
+                         "size to %d pages", QEMU_NFS_MAX_PAGECACHE_SIZE);
+            client->pagecache = QEMU_NFS_MAX_PAGECACHE_SIZE;
+        }
+        nfs_set_pagecache(client->context, client->pagecache);
+        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")) {
+        client->debug = qemu_opt_get_number(opts, "debug", 0);
+        /* limit the maximum debug level to avoid potential flooding
+         * of our log files. */
+        if (client->debug > QEMU_NFS_MAX_DEBUG_LEVEL) {
+            error_report("NFS Warning: Limiting NFS debug level "
+                         "to %d", QEMU_NFS_MAX_DEBUG_LEVEL);
+            client->debug = QEMU_NFS_MAX_DEBUG_LEVEL;
         }
+        nfs_set_debug(client->context, client->debug);
     }
+#endif
 
-    ret = nfs_mount(client->context, uri->server, uri->path);
+    ret = nfs_mount(client->context, client->server->host, client->path);
     if (ret < 0) {
         error_setg(errp, "Failed to mount nfs share: %s",
                    nfs_get_error(client->context));
@@ -413,14 +612,13 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
     ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
     client->st_blocks = st.st_blocks;
     client->has_zero_init = S_ISREG(st.st_mode);
+    sprintf(client->path, "%s%s", client->path, file);
     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;
 }
@@ -429,28 +627,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;
 }
 
@@ -472,6 +659,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();
 
@@ -479,7 +667,13 @@ 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) {
+        goto out;
+    }
+
+    ret = nfs_client_open(client, options, O_CREAT, errp, 0);
     if (ret < 0) {
         goto out;
     }
@@ -561,6 +755,67 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
     return 0;
 }
 
+static void nfs_refresh_filename(BlockDriverState *bs, QDict *options)
+{
+    NFSClient *client = bs->opaque;
+    QDict *opts = qdict_new();
+    QObject *server_qdict;
+    Visitor *ov;
+
+    qdict_put(opts, "driver", qstring_from_str("nfs"));
+
+    if (client->uid && !client->gid) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nfs://%s%s?uid=%" PRId64, client->server->host, client->path,
+                 client->uid);
+    } else if (!client->uid && client->gid) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nfs://%s%s?gid=%" PRId64, client->server->host, client->path,
+                 client->gid);
+    } else if (client->uid && client->gid) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nfs://%s%s?uid=%" PRId64 "&gid=%" PRId64,
+                 client->server->host, client->path, client->uid, client->gid);
+    } else {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nfs://%s%s", client->server->host, client->path);
+    }
+
+    ov = qobject_output_visitor_new(&server_qdict);
+    visit_type_NFSServer(ov, NULL, &client->server, &error_abort);
+    visit_complete(ov, &server_qdict);
+    assert(qobject_type(server_qdict) == QTYPE_QDICT);
+
+    qdict_put_obj(opts, "server", server_qdict);
+    qdict_put(opts, "path", qstring_from_str(client->path));
+
+    if (client->uid) {
+        qdict_put(opts, "uid", qint_from_int(client->uid));
+    }
+    if (client->gid) {
+        qdict_put(opts, "gid", qint_from_int(client->gid));
+    }
+    if (client->tcp_syncnt) {
+        qdict_put(opts, "tcp-syncnt",
+                      qint_from_int(client->tcp_syncnt));
+    }
+    if (client->readahead) {
+        qdict_put(opts, "readahead",
+                      qint_from_int(client->readahead));
+    }
+    if (client->pagecache) {
+        qdict_put(opts, "pagecache",
+                      qint_from_int(client->pagecache));
+    }
+    if (client->debug) {
+        qdict_put(opts, "debug", qint_from_int(client->debug));
+    }
+
+    visit_free(ov);
+    qdict_flatten(opts);
+    bs->full_open_options = opts;
+}
+
 #ifdef LIBNFS_FEATURE_PAGECACHE
 static void nfs_invalidate_cache(BlockDriverState *bs,
                                  Error **errp)
@@ -575,7 +830,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,
@@ -593,6 +848,7 @@ static BlockDriver bdrv_nfs = {
 
     .bdrv_detach_aio_context        = nfs_detach_aio_context,
     .bdrv_attach_aio_context        = nfs_attach_aio_context,
+    .bdrv_refresh_filename          = nfs_refresh_filename,
 
 #ifdef LIBNFS_FEATURE_PAGECACHE
     .bdrv_invalidate_cache          = nfs_invalidate_cache,
-- 
2.6.2

[Qemu-devel] [PATCH v6 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 | 74 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3592a9d..c0c9b48 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1714,14 +1714,14 @@
 #
 # @host_device, @host_cdrom: Since 2.1
 # @gluster: Since 2.7
-# @nbd, @ssh: Since 2.8
+# @nbd, @nfs, @ssh: Since 2.8
 #
 # Since: 2.0
 ##
 { 'enum': 'BlockdevDriver',
   'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
             'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
-            'host_device', 'http', 'https', 'luks', 'nbd', 'null-aio',
+            'host_device', 'http', 'https', 'luks', 'nbd', 'nfs', 'null-aio',
             'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
             'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
             'vvfat' ] }
@@ -2240,6 +2240,74 @@
             '*top-id': 'str' } }
 
 ##
+# @NFSTransport
+#
+# An enumeration of NFS transport types
+#
+# @inet:        TCP transport
+#
+# Since 2.8
+##
+{ 'enum': 'NFSTransport',
+  'data': [ 'inet' ] }
+
+##
+# @NFSServer
+#
+# Captures the address of the socket
+#
+# @type:        transport type used for NFS (only TCP supported)
+#
+# @host:        host address for NFS server
+#
+# Since 2.8
+##
+{ 'struct': 'NFSServer',
+  'data': { 'type': 'NFSTransport',
+            'host': 'str' } }
+
+##
+# @BlockdevOptionsNfs
+#
+# Driver specific block device option for NFS
+#
+# @server:                  host address
+#
+# @path:                    path of the image on the host
+#
+# @user:                    #optional UID value to use when talking to the
+#                           server (defaults to 65534 on Windows and getuid()
+#                           on unix)
+#
+# @group:                   #optional GID value to use when talking to the
+#                           server (defaults to 65534 on Windows and getgid()
+#                           in unix)
+#
+# @tcp-syn-count:           #optional number of SYNs during the session
+#                           establishment (defaults to libnfs default)
+#
+# @readahead-size:          #optional set the readahead size in bytes (defaults
+#                           to libnfs default)
+#
+# @page-cache-size:         #optional set the pagecache size in bytes (defaults
+#                           to libnfs default)
+#
+# @debug-level:             #optional set the NFS debug level (max 2) (defaults
+#                           to libnfs default)
+#
+# Since 2.8
+##
+{ 'struct': 'BlockdevOptionsNfs',
+  'data': { 'server': 'NFSServer',
+            'path': 'str',
+            '*user': 'int',
+            '*group': 'int',
+            '*tcp-syn-count': 'int',
+            '*readahead-size': 'int',
+            '*page-cache-size': 'int',
+            '*debug-level': 'int' } }
+
+##
 # @BlockdevOptionsCurl
 #
 # Driver specific block device options for the curl backend.
@@ -2315,7 +2383,7 @@
 # TODO iscsi: Wait for structured options
       'luks':       'BlockdevOptionsLUKS',
       'nbd':        'BlockdevOptionsNbd',
-# TODO nfs: Wait for structured options
+      'nfs':        'BlockdevOptionsNfs',
       'null-aio':   'BlockdevOptionsNull',
       'null-co':    'BlockdevOptionsNull',
       'parallels':  'BlockdevOptionsGenericFormat',
-- 
2.6.2

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

[Kevin Wolf]

Am 31.10.2016 um 16:05 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.
> Add a new option "server" which then accepts a new struct NFSServer.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>

> @@ -413,14 +612,13 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
>      ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
>      client->st_blocks = st.st_blocks;
>      client->has_zero_init = S_ISREG(st.st_mode);
> +    sprintf(client->path, "%s%s", client->path, file);

This doesn't work. I'm replacing the line with *strp = '/';

Kevin

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Kevin Wolf]

Am 31.10.2016 um 16:05 hat Ashijeet Acharya geschrieben:
> Previously posted series patches:
> v5: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07580.html
> v4: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07449.html
> v3: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06903.html
> v2: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg05844.html
> v1: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04487.html
> 
> This series adds blockdev-add support for NFS block driver.

Thanks, fixed as commented on patch 1 and applied.

Kevin

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Peter Lieven]

Am 31.10.2016 um 18:20 schrieb Kevin Wolf:
> Am 31.10.2016 um 16:05 hat Ashijeet Acharya geschrieben:
>> Previously posted series patches:
>> v5: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07580.html
>> v4: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07449.html
>> v3: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06903.html
>> v2: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg05844.html
>> v1: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04487.html
>>
>> This series adds blockdev-add support for NFS block driver.
> Thanks, fixed as commented on patch 1 and applied.

Hi,

it seems this series breaks passing options via URI.

1) in nfs_parse_uri

parse_uint_full(qp->p[i].value, NULL, 0)

segfaults, as the routine wants to set *NULL = val.

Program received signal SIGSEGV, Segmentation fault.
parse_uint (s=0x555555d0ead0 "131072", value=value@entry=0x0, endptr=endptr@entry=0x7fffffffd870, base=base@entry=0) at util/cutils.c:475
475        *value = val;
(gdb) bt full
#0  parse_uint (s=0x555555d0ead0 "131072", value=value@entry=0x0, endptr=endptr@entry=0x7fffffffd870, base=base@entry=0) at util/cutils.c:475
         r = 0
         endp = 0x555555d0ead6 ""
         val = 131072
#1  0x0000555555655ff2 in parse_uint_full (s=<optimized out>, value=value@entry=0x0, base=base@entry=0) at util/cutils.c:499
         endp = 0x555555d093f0 "\004"
         r = <optimized out>
#2  0x0000555555603425 in nfs_parse_uri (filename=<optimized out>, options=0x555555d093f0, errp=0x7fffffffd980) at block/nfs.c:116
         uri = 0x555555d0e920
         qp = 0x555555d0ea30
         ret = -22
         i = 0
         __func__ = "nfs_parse_uri"
#3  0x000055555559c7bb in bdrv_fill_options (errp=0x7fffffffd968, flags=0x7fffffffd95c, filename=<optimized out>, options=<synthetischer Zeiger>) at block.c:1278
         drvname = <optimized out>
         protocol = <optimized out>
         local_err = 0x0
         parse_filename = true
         drv = 0x5555558e3140 <bdrv_nfs>


2) all parameters that have a different names in options and qdict
e.g. readahead-size vs. readahead cannot be passed via URI.

$ qemu-img convert -p nfs://172.21.200.61/templates/VC_debian8-20170116.qcow2,linux\?readahead=131072 iscsi://172.21.200.56:3260/iqn.2001-05.com.equallogic:0-8a0906-69d384e0a-aa3004e55e15878d-XXX/0

qemu-img: Could not open 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072': Block protocol 'nfs' doesn't support the option 'readahead-size'

Please let me know if the below fix would be correct:

lieven@lieven-pc:~/git/qemu$ git diff
diff --git a/block/nfs.c b/block/nfs.c
index a564340..0ff8268 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -108,30 +108,31 @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
      qdict_put(options, "path", qstring_from_str(uri->path));

      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);
              goto out;
          }
-        if (parse_uint_full(qp->p[i].value, NULL, 0)) {
+        if (parse_uint_full(qp->p[i].value, &val, 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, "user",
+            qdict_put(options, "uid",
                        qstring_from_str(qp->p[i].value));
          } else if (!strcmp(qp->p[i].name, "gid")) {
-            qdict_put(options, "group",
+            qdict_put(options, "gid",
                        qstring_from_str(qp->p[i].value));
          } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
-            qdict_put(options, "tcp-syn-count",
+            qdict_put(options, "tcp-syncnt",
                        qstring_from_str(qp->p[i].value));
          } else if (!strcmp(qp->p[i].name, "readahead")) {
-            qdict_put(options, "readahead-size",
+            qdict_put(options, "readahead",
                        qstring_from_str(qp->p[i].value));
          } else if (!strcmp(qp->p[i].name, "pagecache")) {
-            qdict_put(options, "page-cache-size",
+            qdict_put(options, "pagecache",
                        qstring_from_str(qp->p[i].value));
          } else if (!strcmp(qp->p[i].name, "debug")) {
              qdict_put(options, "debug",


Thanks,
Peter

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Kevin Wolf]

Am 17.01.2017 um 16:14 hat Peter Lieven geschrieben:
> Am 31.10.2016 um 18:20 schrieb Kevin Wolf:
> >Am 31.10.2016 um 16:05 hat Ashijeet Acharya geschrieben:
> >>Previously posted series patches:
> >>v5: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07580.html
> >>v4: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07449.html
> >>v3: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06903.html
> >>v2: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg05844.html
> >>v1: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04487.html
> >>
> >>This series adds blockdev-add support for NFS block driver.
> >Thanks, fixed as commented on patch 1 and applied.
> 
> Hi,
> 
> it seems this series breaks passing options via URI.
> 
> 1) in nfs_parse_uri
> 
> parse_uint_full(qp->p[i].value, NULL, 0)
> 
> segfaults, as the routine wants to set *NULL = val.

Yes, you're right.

> 2) all parameters that have a different names in options and qdict
> e.g. readahead-size vs. readahead cannot be passed via URI.
> 
> $ qemu-img convert -p nfs://172.21.200.61/templates/VC_debian8-20170116.qcow2,linux\?readahead=131072 iscsi://172.21.200.56:3260/iqn.2001-05.com.equallogic:0-8a0906-69d384e0a-aa3004e55e15878d-XXX/0
> 
> qemu-img: Could not open 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072': Block protocol 'nfs' doesn't support the option 'readahead-size'
> 
> Please let me know if the below fix would be correct:

No, this needs to be fixed the other way round: runtime_opts must use
the names as specified in the schema, and nfs_client_open() must access
them as such. Without that, blockdev-add can't work (and the command
line only with the "wrong" old option names from the URL, whereas it
should be using the same names as the QAPI schema).

Kevin

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Peter Lieven]

Am 18.01.2017 um 10:59 schrieb Kevin Wolf:
> Am 17.01.2017 um 16:14 hat Peter Lieven geschrieben:
>> Am 31.10.2016 um 18:20 schrieb Kevin Wolf:
>>> Am 31.10.2016 um 16:05 hat Ashijeet Acharya geschrieben:
>>>> Previously posted series patches:
>>>> v5: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07580.html
>>>> v4: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07449.html
>>>> v3: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06903.html
>>>> v2: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg05844.html
>>>> v1: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04487.html
>>>>
>>>> This series adds blockdev-add support for NFS block driver.
>>> Thanks, fixed as commented on patch 1 and applied.
>> Hi,
>>
>> it seems this series breaks passing options via URI.
>>
>> 1) in nfs_parse_uri
>>
>> parse_uint_full(qp->p[i].value, NULL, 0)
>>
>> segfaults, as the routine wants to set *NULL = val.
> Yes, you're right.
>
>> 2) all parameters that have a different names in options and qdict
>> e.g. readahead-size vs. readahead cannot be passed via URI.
>>
>> $ qemu-img convert -p nfs://172.21.200.61/templates/VC_debian8-20170116.qcow2,linux\?readahead=131072 iscsi://172.21.200.56:3260/iqn.2001-05.com.equallogic:0-8a0906-69d384e0a-aa3004e55e15878d-XXX/0
>>
>> qemu-img: Could not open 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072': Block protocol 'nfs' doesn't support the option 'readahead-size'
>>
>> Please let me know if the below fix would be correct:
> No, this needs to be fixed the other way round: runtime_opts must use
> the names as specified in the schema, and nfs_client_open() must access
> them as such. Without that, blockdev-add can't work (and the command
> line only with the "wrong" old option names from the URL, whereas it
> should be using the same names as the QAPI schema).

Shouldn't we support both for backwards compatiblity.?

Peter

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Eric Blake]

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

On 01/19/2017 08:30 AM, Peter Lieven wrote:
>>> qemu-img: Could not open
>>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
>>> Block protocol 'nfs' doesn't support the option 'readahead-size'
>>>
>>> Please let me know if the below fix would be correct:
>> No, this needs to be fixed the other way round: runtime_opts must use
>> the names as specified in the schema, and nfs_client_open() must access
>> them as such. Without that, blockdev-add can't work (and the command
>> line only with the "wrong" old option names from the URL, whereas it
>> should be using the same names as the QAPI schema).
> 
> Shouldn't we support both for backwards compatiblity.?

blockdev-add only needs to support the modern naming.  But yes,
preserving back-compat spelling of the command-line spellings, as well
as matching blockdev-add spellings, is desirable.

-- 
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 v6 0/2] allow blockdev-add for NFS

[Kevin Wolf]

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

Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
> On 01/19/2017 08:30 AM, Peter Lieven wrote:
> >>> qemu-img: Could not open
> >>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
> >>> Block protocol 'nfs' doesn't support the option 'readahead-size'
> >>>
> >>> Please let me know if the below fix would be correct:
> >> No, this needs to be fixed the other way round: runtime_opts must use
> >> the names as specified in the schema, and nfs_client_open() must access
> >> them as such. Without that, blockdev-add can't work (and the command
> >> line only with the "wrong" old option names from the URL, whereas it
> >> should be using the same names as the QAPI schema).
> > 
> > Shouldn't we support both for backwards compatiblity.?
> 
> blockdev-add only needs to support the modern naming.  But yes,
> preserving back-compat spelling of the command-line spellings, as well
> as matching blockdev-add spellings, is desirable.

We only just added the individual command line options, previously it
only supported the URL.

It's true that we have the messed up version of the options in 2.8, so
strictly speaking we would break compatibility with a release, but it's
only one release, it's only the nfs driver, and the documentation of the
options is the schema, which had the right option names even in 2.8
(they just didn't work).

So I wouldn't feel bad about removing the wrong names in this specific
case.

Kevin

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

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Peter Lieven]

Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
> Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
>> On 01/19/2017 08:30 AM, Peter Lieven wrote:
>>>>> qemu-img: Could not open
>>>>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
>>>>> Block protocol 'nfs' doesn't support the option 'readahead-size'
>>>>>
>>>>> Please let me know if the below fix would be correct:
>>>> No, this needs to be fixed the other way round: runtime_opts must use
>>>> the names as specified in the schema, and nfs_client_open() must access
>>>> them as such. Without that, blockdev-add can't work (and the command
>>>> line only with the "wrong" old option names from the URL, whereas it
>>>> should be using the same names as the QAPI schema).
>>> Shouldn't we support both for backwards compatiblity.?
>> blockdev-add only needs to support the modern naming.  But yes,
>> preserving back-compat spelling of the command-line spellings, as well
>> as matching blockdev-add spellings, is desirable.
> We only just added the individual command line options, previously it
> only supported the URL.
>
> It's true that we have the messed up version of the options in 2.8, so
> strictly speaking we would break compatibility with a release, but it's
> only one release, it's only the nfs driver, and the documentation of the
> options is the schema, which had the right option names even in 2.8
> (they just didn't work).
>
> So I wouldn't feel bad about removing the wrong names in this specific
> case.

So want exactly do you want to do? Fix the names in the QAPI schema
to use the old naming?

Peter

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Kevin Wolf]

Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
> Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
> >Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
> >>On 01/19/2017 08:30 AM, Peter Lieven wrote:
> >>>>>qemu-img: Could not open
> >>>>>'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
> >>>>>Block protocol 'nfs' doesn't support the option 'readahead-size'
> >>>>>
> >>>>>Please let me know if the below fix would be correct:
> >>>>No, this needs to be fixed the other way round: runtime_opts must use
> >>>>the names as specified in the schema, and nfs_client_open() must access
> >>>>them as such. Without that, blockdev-add can't work (and the command
> >>>>line only with the "wrong" old option names from the URL, whereas it
> >>>>should be using the same names as the QAPI schema).
> >>>Shouldn't we support both for backwards compatiblity.?
> >>blockdev-add only needs to support the modern naming.  But yes,
> >>preserving back-compat spelling of the command-line spellings, as well
> >>as matching blockdev-add spellings, is desirable.
> >We only just added the individual command line options, previously it
> >only supported the URL.
> >
> >It's true that we have the messed up version of the options in 2.8, so
> >strictly speaking we would break compatibility with a release, but it's
> >only one release, it's only the nfs driver, and the documentation of the
> >options is the schema, which had the right option names even in 2.8
> >(they just didn't work).
> >
> >So I wouldn't feel bad about removing the wrong names in this specific
> >case.
> 
> So want exactly do you want to do? Fix the names in the QAPI schema
> to use the old naming?

No, fix the command line to use the names in the QAPI schema.

The option names from the URL were never supposed to be supported on the
command line.

Kevin

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Peter Lieven]

Am 19.01.2017 um 16:42 schrieb Kevin Wolf:
> Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
>> Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
>>> Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
>>>> On 01/19/2017 08:30 AM, Peter Lieven wrote:
>>>>>>> qemu-img: Could not open
>>>>>>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
>>>>>>> Block protocol 'nfs' doesn't support the option 'readahead-size'
>>>>>>>
>>>>>>> Please let me know if the below fix would be correct:
>>>>>> No, this needs to be fixed the other way round: runtime_opts must use
>>>>>> the names as specified in the schema, and nfs_client_open() must access
>>>>>> them as such. Without that, blockdev-add can't work (and the command
>>>>>> line only with the "wrong" old option names from the URL, whereas it
>>>>>> should be using the same names as the QAPI schema).
>>>>> Shouldn't we support both for backwards compatiblity.?
>>>> blockdev-add only needs to support the modern naming.  But yes,
>>>> preserving back-compat spelling of the command-line spellings, as well
>>>> as matching blockdev-add spellings, is desirable.
>>> We only just added the individual command line options, previously it
>>> only supported the URL.
>>>
>>> It's true that we have the messed up version of the options in 2.8, so
>>> strictly speaking we would break compatibility with a release, but it's
>>> only one release, it's only the nfs driver, and the documentation of the
>>> options is the schema, which had the right option names even in 2.8
>>> (they just didn't work).
>>>
>>> So I wouldn't feel bad about removing the wrong names in this specific
>>> case.
>> So want exactly do you want to do? Fix the names in the QAPI schema
>> to use the old naming?
> No, fix the command line to use the names in the QAPI schema.
>
> The option names from the URL were never supposed to be supported on the
> command line.

Okay, so no backwards compatiblity? I actually used the options on the command line...

Peter

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Peter Lieven]

Am 19.01.2017 um 16:42 schrieb Kevin Wolf:
> Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
>> Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
>>> Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
>>>> On 01/19/2017 08:30 AM, Peter Lieven wrote:
>>>>>>> qemu-img: Could not open
>>>>>>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
>>>>>>> Block protocol 'nfs' doesn't support the option 'readahead-size'
>>>>>>>
>>>>>>> Please let me know if the below fix would be correct:
>>>>>> No, this needs to be fixed the other way round: runtime_opts must use
>>>>>> the names as specified in the schema, and nfs_client_open() must access
>>>>>> them as such. Without that, blockdev-add can't work (and the command
>>>>>> line only with the "wrong" old option names from the URL, whereas it
>>>>>> should be using the same names as the QAPI schema).
>>>>> Shouldn't we support both for backwards compatiblity.?
>>>> blockdev-add only needs to support the modern naming.  But yes,
>>>> preserving back-compat spelling of the command-line spellings, as well
>>>> as matching blockdev-add spellings, is desirable.
>>> We only just added the individual command line options, previously it
>>> only supported the URL.
>>>
>>> It's true that we have the messed up version of the options in 2.8, so
>>> strictly speaking we would break compatibility with a release, but it's
>>> only one release, it's only the nfs driver, and the documentation of the
>>> options is the schema, which had the right option names even in 2.8
>>> (they just didn't work).
>>>
>>> So I wouldn't feel bad about removing the wrong names in this specific
>>> case.
>> So want exactly do you want to do? Fix the names in the QAPI schema
>> to use the old naming?
> No, fix the command line to use the names in the QAPI schema.
>
> The option names from the URL were never supposed to be supported on the
> command line.
>
> Kevin

This is what I wanted to do (before I asked ;-)):

diff --git a/block/nfs.c b/block/nfs.c
index baaecff..7c997cd 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -119,19 +119,24 @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
                         qp->p[i].name);
              goto out;
          }
-        if (!strcmp(qp->p[i].name, "uid")) {
+        if (!strcmp(qp->p[i].name, "uid") ||
+            !strcmp(qp->p[i].name, "user")) {
              qdict_put(options, "user",
                        qstring_from_str(qp->p[i].value));
-        } else if (!strcmp(qp->p[i].name, "gid")) {
+        } else if (!strcmp(qp->p[i].name, "gid") ||
+                   !strcmp(qp->p[i].name, "group")) {
              qdict_put(options, "group",
                        qstring_from_str(qp->p[i].value));
-        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
+        } else if (!strcmp(qp->p[i].name, "tcp-syncnt") ||
+                   !strcmp(qp->p[i].name, "tcp-syn-count")) {
              qdict_put(options, "tcp-syn-count",
                        qstring_from_str(qp->p[i].value));
-        } else if (!strcmp(qp->p[i].name, "readahead")) {
+        } else if (!strcmp(qp->p[i].name, "readahead") ||
+                   !strcmp(qp->p[i].name, "readahead-size")) {
              qdict_put(options, "readahead-size",
                        qstring_from_str(qp->p[i].value));
-        } else if (!strcmp(qp->p[i].name, "pagecache")) {
+        } else if (!strcmp(qp->p[i].name, "pagecache") ||
+                   !strcmp(qp->p[i].name, "page-cache-size")) {
              qdict_put(options, "page-cache-size",
                        qstring_from_str(qp->p[i].value));
          } else if (!strcmp(qp->p[i].name, "debug")) {
@@ -359,27 +364,27 @@ static QemuOptsList runtime_opts = {
              .help = "Path of the image on the host",
          },
          {
-            .name = "uid",
+            .name = "user",
              .type = QEMU_OPT_NUMBER,
              .help = "UID value to use when talking to the server",
          },
          {
-            .name = "gid",
+            .name = "group",
              .type = QEMU_OPT_NUMBER,
              .help = "GID value to use when talking to the server",
          },
          {
-            .name = "tcp-syncnt",
+            .name = "tcp-syn-count",
              .type = QEMU_OPT_NUMBER,
              .help = "Number of SYNs to send during the session establish",
          },
          {
-            .name = "readahead",
+            .name = "readahead-size",
              .type = QEMU_OPT_NUMBER,
              .help = "Set the readahead size in bytes",
          },
          {
-            .name = "pagecache",
+            .name = "page-cache-size",
              .type = QEMU_OPT_NUMBER,
              .help = "Set the pagecache size in bytes",
          },
@@ -508,29 +513,29 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options,
          goto fail;
      }

-    if (qemu_opt_get(opts, "uid")) {
-        client->uid = qemu_opt_get_number(opts, "uid", 0);
+    if (qemu_opt_get(opts, "user")) {
+        client->uid = qemu_opt_get_number(opts, "user", 0);
          nfs_set_uid(client->context, client->uid);
      }

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

-    if (qemu_opt_get(opts, "tcp-syncnt")) {
-        client->tcp_syncnt = qemu_opt_get_number(opts, "tcp-syncnt", 0);
+    if (qemu_opt_get(opts, "tcp-syn-count")) {
+        client->tcp_syncnt = qemu_opt_get_number(opts, "tcp-syn-count", 0);
          nfs_set_tcp_syncnt(client->context, client->tcp_syncnt);
      }

  #ifdef LIBNFS_FEATURE_READAHEAD
-    if (qemu_opt_get(opts, "readahead")) {
+    if (qemu_opt_get(opts, "readahead-size")) {
          if (open_flags & BDRV_O_NOCACHE) {
              error_setg(errp, "Cannot enable NFS readahead "
                               "if cache.direct = on");
              goto fail;
          }
-        client->readahead = qemu_opt_get_number(opts, "readahead", 0);
+        client->readahead = qemu_opt_get_number(opts, "readahead-size", 0);
          if (client->readahead > QEMU_NFS_MAX_READAHEAD_SIZE) {
              error_report("NFS Warning: Truncating NFS readahead "
                           "size to %d", QEMU_NFS_MAX_READAHEAD_SIZE);
@@ -545,13 +550,13 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options,
  #endif

  #ifdef LIBNFS_FEATURE_PAGECACHE
-    if (qemu_opt_get(opts, "pagecache")) {
+    if (qemu_opt_get(opts, "page-cache-size")) {
          if (open_flags & BDRV_O_NOCACHE) {
              error_setg(errp, "Cannot enable NFS pagecache "
                               "if cache.direct = on");
              goto fail;
          }
-        client->pagecache = qemu_opt_get_number(opts, "pagecache", 0);
+        client->pagecache = qemu_opt_get_number(opts, "page-cache-size", 0);
          if (client->pagecache > QEMU_NFS_MAX_PAGECACHE_SIZE) {
              error_report("NFS Warning: Truncating NFS pagecache "
                           "size to %d pages", QEMU_NFS_MAX_PAGECACHE_SIZE);


Peter

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Kevin Wolf]

Am 19.01.2017 um 16:44 hat Peter Lieven geschrieben:
> Am 19.01.2017 um 16:42 schrieb Kevin Wolf:
> >Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
> >>Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
> >>>Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
> >>>>On 01/19/2017 08:30 AM, Peter Lieven wrote:
> >>>>>>>qemu-img: Could not open
> >>>>>>>'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
> >>>>>>>Block protocol 'nfs' doesn't support the option 'readahead-size'
> >>>>>>>
> >>>>>>>Please let me know if the below fix would be correct:
> >>>>>>No, this needs to be fixed the other way round: runtime_opts must use
> >>>>>>the names as specified in the schema, and nfs_client_open() must access
> >>>>>>them as such. Without that, blockdev-add can't work (and the command
> >>>>>>line only with the "wrong" old option names from the URL, whereas it
> >>>>>>should be using the same names as the QAPI schema).
> >>>>>Shouldn't we support both for backwards compatiblity.?
> >>>>blockdev-add only needs to support the modern naming.  But yes,
> >>>>preserving back-compat spelling of the command-line spellings, as well
> >>>>as matching blockdev-add spellings, is desirable.
> >>>We only just added the individual command line options, previously it
> >>>only supported the URL.
> >>>
> >>>It's true that we have the messed up version of the options in 2.8, so
> >>>strictly speaking we would break compatibility with a release, but it's
> >>>only one release, it's only the nfs driver, and the documentation of the
> >>>options is the schema, which had the right option names even in 2.8
> >>>(they just didn't work).
> >>>
> >>>So I wouldn't feel bad about removing the wrong names in this specific
> >>>case.
> >>So want exactly do you want to do? Fix the names in the QAPI schema
> >>to use the old naming?
> >No, fix the command line to use the names in the QAPI schema.
> >
> >The option names from the URL were never supposed to be supported on the
> >command line.
> 
> Okay, so no backwards compatiblity? I actually used the options on the command line...

Well, do you _need_ compatibility?

It can certainly be done, but as the (wrong) options on the command line
have only existed since November and were never documented, I wouldn't
bother unless there's a good reason.

Kevin

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Peter Lieven]

Am 19.01.2017 um 16:55 schrieb Kevin Wolf:
> Am 19.01.2017 um 16:44 hat Peter Lieven geschrieben:
>> Am 19.01.2017 um 16:42 schrieb Kevin Wolf:
>>> Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
>>>> Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
>>>>> Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
>>>>>> On 01/19/2017 08:30 AM, Peter Lieven wrote:
>>>>>>>>> qemu-img: Could not open
>>>>>>>>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
>>>>>>>>> Block protocol 'nfs' doesn't support the option 'readahead-size'
>>>>>>>>>
>>>>>>>>> Please let me know if the below fix would be correct:
>>>>>>>> No, this needs to be fixed the other way round: runtime_opts must use
>>>>>>>> the names as specified in the schema, and nfs_client_open() must access
>>>>>>>> them as such. Without that, blockdev-add can't work (and the command
>>>>>>>> line only with the "wrong" old option names from the URL, whereas it
>>>>>>>> should be using the same names as the QAPI schema).
>>>>>>> Shouldn't we support both for backwards compatiblity.?
>>>>>> blockdev-add only needs to support the modern naming.  But yes,
>>>>>> preserving back-compat spelling of the command-line spellings, as well
>>>>>> as matching blockdev-add spellings, is desirable.
>>>>> We only just added the individual command line options, previously it
>>>>> only supported the URL.
>>>>>
>>>>> It's true that we have the messed up version of the options in 2.8, so
>>>>> strictly speaking we would break compatibility with a release, but it's
>>>>> only one release, it's only the nfs driver, and the documentation of the
>>>>> options is the schema, which had the right option names even in 2.8
>>>>> (they just didn't work).
>>>>>
>>>>> So I wouldn't feel bad about removing the wrong names in this specific
>>>>> case.
>>>> So want exactly do you want to do? Fix the names in the QAPI schema
>>>> to use the old naming?
>>> No, fix the command line to use the names in the QAPI schema.
>>>
>>> The option names from the URL were never supposed to be supported on the
>>> command line.
>> Okay, so no backwards compatiblity? I actually used the options on the command line...
> Well, do you _need_ compatibility?
>
> It can certainly be done, but as the (wrong) options on the command line
> have only existed since November and were never documented, I wouldn't
> bother unless there's a good reason.

Every Qemu before 2.8.0 was working with sth like:

qemu -cdrom nfs://10.0.0.1/expory/my.iso?readahead=131072

Peter

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Kevin Wolf]

Am 19.01.2017 um 16:58 hat Peter Lieven geschrieben:
> Am 19.01.2017 um 16:55 schrieb Kevin Wolf:
> >Am 19.01.2017 um 16:44 hat Peter Lieven geschrieben:
> >>Am 19.01.2017 um 16:42 schrieb Kevin Wolf:
> >>>Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
> >>>>Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
> >>>>>Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
> >>>>>>On 01/19/2017 08:30 AM, Peter Lieven wrote:
> >>>>>>>>>qemu-img: Could not open
> >>>>>>>>>'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
> >>>>>>>>>Block protocol 'nfs' doesn't support the option 'readahead-size'
> >>>>>>>>>
> >>>>>>>>>Please let me know if the below fix would be correct:
> >>>>>>>>No, this needs to be fixed the other way round: runtime_opts must use
> >>>>>>>>the names as specified in the schema, and nfs_client_open() must access
> >>>>>>>>them as such. Without that, blockdev-add can't work (and the command
> >>>>>>>>line only with the "wrong" old option names from the URL, whereas it
> >>>>>>>>should be using the same names as the QAPI schema).
> >>>>>>>Shouldn't we support both for backwards compatiblity.?
> >>>>>>blockdev-add only needs to support the modern naming.  But yes,
> >>>>>>preserving back-compat spelling of the command-line spellings, as well
> >>>>>>as matching blockdev-add spellings, is desirable.
> >>>>>We only just added the individual command line options, previously it
> >>>>>only supported the URL.
> >>>>>
> >>>>>It's true that we have the messed up version of the options in 2.8, so
> >>>>>strictly speaking we would break compatibility with a release, but it's
> >>>>>only one release, it's only the nfs driver, and the documentation of the
> >>>>>options is the schema, which had the right option names even in 2.8
> >>>>>(they just didn't work).
> >>>>>
> >>>>>So I wouldn't feel bad about removing the wrong names in this specific
> >>>>>case.
> >>>>So want exactly do you want to do? Fix the names in the QAPI schema
> >>>>to use the old naming?
> >>>No, fix the command line to use the names in the QAPI schema.
> >>>
> >>>The option names from the URL were never supposed to be supported on the
> >>>command line.
> >>Okay, so no backwards compatiblity? I actually used the options on the command line...
> >Well, do you _need_ compatibility?
> >
> >It can certainly be done, but as the (wrong) options on the command line
> >have only existed since November and were never documented, I wouldn't
> >bother unless there's a good reason.
> 
> Every Qemu before 2.8.0 was working with sth like:
> 
> qemu -cdrom nfs://10.0.0.1/expory/my.iso?readahead=131072

That will keep working. We're not changing the URL parsing, just the
runtime_opts and its accesses in nfs_client_open(). The translation in
nfs_parse_uri() stays intact with the fixes.

What will stop working (and only worked in 2.8.0) is this:

    qemu -drive media=cdrom,driver=nfs,server.host=10.0.0.1,path=export/my.iso,readahead=131072

Also, I think the fixes should be Cc: qemu-stable, so that 2.8.1 will
work correctly again.

Kevin

Re: [Qemu-devel] [PATCH v6 0/2] allow blockdev-add for NFS

[Peter Lieven]

Am 19.01.2017 um 18:08 schrieb Kevin Wolf:
> Am 19.01.2017 um 16:58 hat Peter Lieven geschrieben:
>> Am 19.01.2017 um 16:55 schrieb Kevin Wolf:
>>> Am 19.01.2017 um 16:44 hat Peter Lieven geschrieben:
>>>> Am 19.01.2017 um 16:42 schrieb Kevin Wolf:
>>>>> Am 19.01.2017 um 16:34 hat Peter Lieven geschrieben:
>>>>>> Am 19.01.2017 um 16:20 schrieb Kevin Wolf:
>>>>>>> Am 19.01.2017 um 15:59 hat Eric Blake geschrieben:
>>>>>>>> On 01/19/2017 08:30 AM, Peter Lieven wrote:
>>>>>>>>>>> qemu-img: Could not open
>>>>>>>>>>> 'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
>>>>>>>>>>> Block protocol 'nfs' doesn't support the option 'readahead-size'
>>>>>>>>>>>
>>>>>>>>>>> Please let me know if the below fix would be correct:
>>>>>>>>>> No, this needs to be fixed the other way round: runtime_opts must use
>>>>>>>>>> the names as specified in the schema, and nfs_client_open() must access
>>>>>>>>>> them as such. Without that, blockdev-add can't work (and the command
>>>>>>>>>> line only with the "wrong" old option names from the URL, whereas it
>>>>>>>>>> should be using the same names as the QAPI schema).
>>>>>>>>> Shouldn't we support both for backwards compatiblity.?
>>>>>>>> blockdev-add only needs to support the modern naming.  But yes,
>>>>>>>> preserving back-compat spelling of the command-line spellings, as well
>>>>>>>> as matching blockdev-add spellings, is desirable.
>>>>>>> We only just added the individual command line options, previously it
>>>>>>> only supported the URL.
>>>>>>>
>>>>>>> It's true that we have the messed up version of the options in 2.8, so
>>>>>>> strictly speaking we would break compatibility with a release, but it's
>>>>>>> only one release, it's only the nfs driver, and the documentation of the
>>>>>>> options is the schema, which had the right option names even in 2.8
>>>>>>> (they just didn't work).
>>>>>>>
>>>>>>> So I wouldn't feel bad about removing the wrong names in this specific
>>>>>>> case.
>>>>>> So want exactly do you want to do? Fix the names in the QAPI schema
>>>>>> to use the old naming?
>>>>> No, fix the command line to use the names in the QAPI schema.
>>>>>
>>>>> The option names from the URL were never supposed to be supported on the
>>>>> command line.
>>>> Okay, so no backwards compatiblity? I actually used the options on the command line...
>>> Well, do you _need_ compatibility?
>>>
>>> It can certainly be done, but as the (wrong) options on the command line
>>> have only existed since November and were never documented, I wouldn't
>>> bother unless there's a good reason.
>> Every Qemu before 2.8.0 was working with sth like:
>>
>> qemu -cdrom nfs://10.0.0.1/expory/my.iso?readahead=131072
> That will keep working. We're not changing the URL parsing, just the
> runtime_opts and its accesses in nfs_client_open(). The translation in
> nfs_parse_uri() stays intact with the fixes.
>
> What will stop working (and only worked in 2.8.0) is this:
>
>      qemu -drive media=cdrom,driver=nfs,server.host=10.0.0.1,path=export/my.iso,readahead=131072
>
> Also, I think the fixes should be Cc: qemu-stable, so that 2.8.1 will
> work correctly again.

Okay, I hope I got it. Will send a patch tomorrow.

Peter