* [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS
@ 2014-01-29 8:50 Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 1/5] " Peter Lieven
` (5 more replies)
0 siblings, 6 replies; 23+ messages in thread
From: Peter Lieven @ 2014-01-29 8:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, ronniesahlberg, Jeff Cody, Peter Lieven,
Max Reitz, owasserm, Federico Simoncelli, Stefan Hajnoczi,
Wenchao Xia
This adds v6 of the NFS protocol driver + qemu-iotest adjustments.
v6->v7:
- removed unsused and partially non-existing libnfs header file includes [Stefan]
- adjusted usage of qemu_opts_create_nofail [Stefan]
- removed patch to fix output of qemu-iotest 067
- only blacklist qemu-iotest 020 for NFS do not change proto from generic to file [Fam]
v5->v6:
- use internal qemu function to parse the NFS url [Kevin]
- zero pad short reads [Kevin, Paolo]
- added qemu-iotests patches for basic nfs protocol support
v4->v5:
- disussed with Ronnie and decided to move URL + Paramter parsing to LibNFS.
This allows for URL parameter processing directly in LibNFS without altering
the qemu NFS block driver. This bumps the version requirement for LibNFS
to 1.9.0 though.
- added a pointer to the LibNFS readme where additional information about
ROOT privilidge requirements can be found as this raised a few concerns.
- removed a trailing dot in an error statement [Fam].
v3->v4:
- finally added full implementation of bdrv_get_allocated_file_size [Stefan]
- removed trailing \n from error statements [Stefan]
v2->v3:
- rebased the stefanha/block
- use pkg_config to check for libnfs (ignoring cflags which are broken in 1.8.0) [Stefan]
- fixed NFSClient declaration [Stefan]
- renamed Task variables to task [Stefan]
- renamed NFSTask to NFSRPC [Ronnie]
- do not update bs->total_sectors in nfs_co_writev [Stefan]
- return -ENOMEM on all async call failures [Stefan,Ronnie]
- fully implement ftruncate
- use util/uri.c for URL parsing [Stefan]
- reworked nfs_file_open_common to nfs_client_open which works on NFSClient [Stefan]
- added a comment ot the connect message that libnfs support NFSv3 only at the moment.
- DID NOT add full implementation of bdrv_get_allocated_file_size because
we are not in a coroutine context and I cannot do an async call here.
I could do a sync call if there would be a guarantee that no requests
are in flight. [Stefan]
v1->v2:
- fixed block/Makefile.objs [Ronnie]
- do not always register a read handler [Ronnie]
- add support for reading beyond EOF [Fam]
- fixed struct and paramter naming [Fam]
- fixed overlong lines and whitespace errors [Fam]
- return return status from libnfs whereever possible [Fam]
- added comment why we set allocated_file_size to -ENOTSUP after write [Fam]
- avoid segfault when parsing filname [Fam]
- remove unused close_bh from NFSClient [Fam]
- avoid dividing and mutliplying total_size by BDRV_SECTOR_SIZE in nfs_file_create [Fam]
Peter Lieven (5):
block: add native support for NFS
qemu-iotests: change _supported_proto to file for various tests
qemu-iotests: enable support for NFS protocol
qemu-iotests: enable test 016 and 025 to work with NFS protocol
qemu-iotests: blacklist test 020 for NFS protocol
MAINTAINERS | 5 +
block/Makefile.objs | 1 +
block/nfs.c | 440 ++++++++++++++++++++++++++++++++++++++++++
configure | 26 +++
qapi-schema.json | 1 +
tests/qemu-iotests/013 | 2 +-
tests/qemu-iotests/014 | 2 +-
tests/qemu-iotests/016 | 2 +-
tests/qemu-iotests/018 | 2 +-
tests/qemu-iotests/019 | 2 +-
tests/qemu-iotests/020 | 5 +
tests/qemu-iotests/023 | 2 +-
tests/qemu-iotests/024 | 2 +-
tests/qemu-iotests/025 | 2 +-
tests/qemu-iotests/026 | 2 +-
tests/qemu-iotests/028 | 2 +-
tests/qemu-iotests/031 | 2 +-
tests/qemu-iotests/034 | 2 +-
tests/qemu-iotests/036 | 2 +-
tests/qemu-iotests/037 | 2 +-
tests/qemu-iotests/038 | 2 +-
tests/qemu-iotests/039 | 2 +-
tests/qemu-iotests/043 | 2 +-
tests/qemu-iotests/046 | 2 +-
tests/qemu-iotests/052 | 2 +-
tests/qemu-iotests/054 | 2 +-
tests/qemu-iotests/059 | 2 +-
tests/qemu-iotests/060 | 2 +-
tests/qemu-iotests/061 | 2 +-
tests/qemu-iotests/063 | 2 +-
tests/qemu-iotests/069 | 2 +-
tests/qemu-iotests/common | 22 ++-
tests/qemu-iotests/common.rc | 3 +
33 files changed, 525 insertions(+), 28 deletions(-)
create mode 100644 block/nfs.c
--
1.7.9.5
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-29 8:50 [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Peter Lieven
@ 2014-01-29 8:50 ` Peter Lieven
2014-01-29 16:19 ` Benoît Canet
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 2/5] qemu-iotests: change _supported_proto to file for various tests Peter Lieven
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2014-01-29 8:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, ronniesahlberg, Jeff Cody, Peter Lieven,
Max Reitz, owasserm, Federico Simoncelli, Stefan Hajnoczi,
Wenchao Xia
This patch adds native support for accessing images on NFS
shares without the requirement to actually mount the entire
NFS share on the host.
NFS Images can simply be specified by an url of the form:
nfs://<host>/<export>/<filename>[?param=value[¶m2=value2[&...]]]
For example:
qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2
You need LibNFS from Ronnie Sahlberg available at:
git://github.com/sahlberg/libnfs.git
for this to work.
During configure it is automatically probed for libnfs and support
is enabled on-the-fly. You can forbid or enforce libnfs support
with --disable-libnfs or --enable-libnfs respectively.
Due to NFS restrictions you might need to execute your binaries
as root, allow them to open priviledged ports (<1024) or specify
insecure option on the NFS server.
For additional information on ROOT vs. non-ROOT operation and URL
format + parameters see:
https://raw.github.com/sahlberg/libnfs/master/README
Supported by qemu are the uid, gid and tcp-syncnt URL parameters.
LibNFS currently support NFS version 3 only.
Signed-off-by: Peter Lieven <pl@kamp.de>
---
MAINTAINERS | 5 +
block/Makefile.objs | 1 +
block/nfs.c | 440 +++++++++++++++++++++++++++++++++++++++++++++++++++
configure | 26 +++
qapi-schema.json | 1 +
5 files changed, 473 insertions(+)
create mode 100644 block/nfs.c
diff --git a/MAINTAINERS b/MAINTAINERS
index fb53242..f8411f9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -936,6 +936,11 @@ M: Peter Lieven <pl@kamp.de>
S: Supported
F: block/iscsi.c
+NFS
+M: Peter Lieven <pl@kamp.de>
+S: Maintained
+F: block/nfs.c
+
SSH
M: Richard W.M. Jones <rjones@redhat.com>
S: Supported
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 4e8c91e..e254a21 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
ifeq ($(CONFIG_POSIX),y)
block-obj-y += nbd.o nbd-client.o sheepdog.o
block-obj-$(CONFIG_LIBISCSI) += iscsi.o
+block-obj-$(CONFIG_LIBNFS) += nfs.o
block-obj-$(CONFIG_CURL) += curl.o
block-obj-$(CONFIG_RBD) += rbd.o
block-obj-$(CONFIG_GLUSTERFS) += gluster.o
diff --git a/block/nfs.c b/block/nfs.c
new file mode 100644
index 0000000..2bbf7a2
--- /dev/null
+++ b/block/nfs.c
@@ -0,0 +1,440 @@
+/*
+ * QEMU Block driver for native access to files on NFS shares
+ *
+ * Copyright (c) 2014 Peter Lieven <pl@kamp.de>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "config-host.h"
+
+#include <poll.h>
+#include "qemu-common.h"
+#include "qemu/config-file.h"
+#include "qemu/error-report.h"
+#include "block/block_int.h"
+#include "trace.h"
+#include "qemu/iov.h"
+#include "qemu/uri.h"
+#include "sysemu/sysemu.h"
+#include <nfsc/libnfs.h>
+
+typedef struct NFSClient {
+ struct nfs_context *context;
+ struct nfsfh *fh;
+ int events;
+ bool has_zero_init;
+} NFSClient;
+
+typedef struct NFSRPC {
+ int status;
+ int complete;
+ QEMUIOVector *iov;
+ struct stat *st;
+ Coroutine *co;
+ QEMUBH *bh;
+} NFSRPC;
+
+static void nfs_process_read(void *arg);
+static void nfs_process_write(void *arg);
+
+static void nfs_set_events(NFSClient *client)
+{
+ int ev = nfs_which_events(client->context);
+ if (ev != client->events) {
+ qemu_aio_set_fd_handler(nfs_get_fd(client->context),
+ (ev & POLLIN) ? nfs_process_read : NULL,
+ (ev & POLLOUT) ? nfs_process_write : NULL,
+ client);
+
+ }
+ client->events = ev;
+}
+
+static void nfs_process_read(void *arg)
+{
+ NFSClient *client = arg;
+ nfs_service(client->context, POLLIN);
+ nfs_set_events(client);
+}
+
+static void nfs_process_write(void *arg)
+{
+ NFSClient *client = arg;
+ nfs_service(client->context, POLLOUT);
+ nfs_set_events(client);
+}
+
+static void nfs_co_init_task(NFSClient *client, NFSRPC *task)
+{
+ *task = (NFSRPC) {
+ .co = qemu_coroutine_self(),
+ };
+}
+
+static void nfs_co_generic_bh_cb(void *opaque)
+{
+ NFSRPC *task = opaque;
+ qemu_bh_delete(task->bh);
+ qemu_coroutine_enter(task->co, NULL);
+}
+
+static void
+nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
+ void *private_data)
+{
+ NFSRPC *task = private_data;
+ task->complete = 1;
+ task->status = status;
+ if (task->status > 0 && task->iov) {
+ if (task->status <= task->iov->size) {
+ qemu_iovec_from_buf(task->iov, 0, data, task->status);
+ } else {
+ task->status = -EIO;
+ }
+ }
+ if (task->status == 0 && task->st) {
+ memcpy(task->st, data, sizeof(struct stat));
+ }
+ if (task->co) {
+ task->bh = qemu_bh_new(nfs_co_generic_bh_cb, task);
+ qemu_bh_schedule(task->bh);
+ }
+}
+
+static int coroutine_fn nfs_co_readv(BlockDriverState *bs,
+ int64_t sector_num, int nb_sectors,
+ QEMUIOVector *iov)
+{
+ NFSClient *client = bs->opaque;
+ NFSRPC task;
+
+ nfs_co_init_task(client, &task);
+ task.iov = iov;
+
+ if (nfs_pread_async(client->context, client->fh,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
+ nfs_co_generic_cb, &task) != 0) {
+ return -ENOMEM;
+ }
+
+ while (!task.complete) {
+ nfs_set_events(client);
+ qemu_coroutine_yield();
+ }
+
+ if (task.status < 0) {
+ return task.status;
+ }
+
+ /* zero pad short reads */
+ if (task.status < iov->size) {
+ qemu_iovec_memset(iov, task.status, 0, iov->size - task.status);
+ }
+
+ return 0;
+}
+
+static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
+ int64_t sector_num, int nb_sectors,
+ QEMUIOVector *iov)
+{
+ NFSClient *client = bs->opaque;
+ NFSRPC task;
+ char *buf = NULL;
+
+ nfs_co_init_task(client, &task);
+
+ buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
+ qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
+
+ if (nfs_pwrite_async(client->context, client->fh,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
+ buf, nfs_co_generic_cb, &task) != 0) {
+ g_free(buf);
+ return -ENOMEM;
+ }
+
+ while (!task.complete) {
+ nfs_set_events(client);
+ qemu_coroutine_yield();
+ }
+
+ g_free(buf);
+
+ if (task.status != nb_sectors * BDRV_SECTOR_SIZE) {
+ return task.status < 0 ? task.status : -EIO;
+ }
+
+ return 0;
+}
+
+static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
+{
+ NFSClient *client = bs->opaque;
+ NFSRPC task;
+
+ nfs_co_init_task(client, &task);
+
+ if (nfs_fsync_async(client->context, client->fh, nfs_co_generic_cb,
+ &task) != 0) {
+ return -ENOMEM;
+ }
+
+ while (!task.complete) {
+ nfs_set_events(client);
+ qemu_coroutine_yield();
+ }
+
+ return task.status;
+}
+
+/* TODO Convert to fine grained options */
+static QemuOptsList runtime_opts = {
+ .name = "nfs",
+ .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+ .desc = {
+ {
+ .name = "filename",
+ .type = QEMU_OPT_STRING,
+ .help = "URL to the NFS file",
+ },
+ { /* end of list */ }
+ },
+};
+
+static void nfs_client_close(NFSClient *client)
+{
+ if (client->context) {
+ if (client->fh) {
+ nfs_close(client->context, client->fh);
+ }
+ qemu_aio_set_fd_handler(nfs_get_fd(client->context), NULL, NULL, NULL);
+ nfs_destroy_context(client->context);
+ }
+ memset(client, 0, sizeof(NFSClient));
+}
+
+static void nfs_file_close(BlockDriverState *bs)
+{
+ NFSClient *client = bs->opaque;
+ nfs_client_close(client);
+}
+
+static int64_t nfs_client_open(NFSClient *client, const char *filename,
+ int flags, Error **errp)
+{
+ int ret = -EINVAL, i;
+ 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");
+ goto fail;
+ }
+ strp = strrchr(uri->path, '/');
+ if (strp == NULL) {
+ error_setg(errp, "Invalid URL specified");
+ goto fail;
+ }
+ file = g_strdup(strp);
+ *strp = 0;
+
+ client->context = nfs_init_context();
+ if (client->context == NULL) {
+ error_setg(errp, "Failed to init NFS context");
+ goto fail;
+ }
+
+ qp = query_params_parse(uri->query);
+ 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 fail;
+ }
+ if (!strncmp(qp->p[i].name, "uid", 3)) {
+ nfs_set_uid(client->context, atoi(qp->p[i].value));
+ } else if (!strncmp(qp->p[i].name, "gid", 3)) {
+ nfs_set_gid(client->context, atoi(qp->p[i].value));
+ } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
+ nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
+ } else {
+ error_setg(errp, "Unknown NFS parameter name: %s",
+ qp->p[i].name);
+ goto fail;
+ }
+ }
+
+ ret = nfs_mount(client->context, uri->server, uri->path);
+ if (ret < 0) {
+ error_setg(errp, "Failed to mount nfs share: %s",
+ nfs_get_error(client->context));
+ goto fail;
+ }
+
+ if (flags & O_CREAT) {
+ ret = nfs_creat(client->context, file, 0600, &client->fh);
+ if (ret < 0) {
+ error_setg(errp, "Failed to create file: %s",
+ nfs_get_error(client->context));
+ goto fail;
+ }
+ } else {
+ ret = nfs_open(client->context, file, flags, &client->fh);
+ if (ret < 0) {
+ error_setg(errp, "Failed to open file : %s",
+ nfs_get_error(client->context));
+ goto fail;
+ }
+ }
+
+ ret = nfs_fstat(client->context, client->fh, &st);
+ if (ret < 0) {
+ error_setg(errp, "Failed to fstat file: %s",
+ nfs_get_error(client->context));
+ goto fail;
+ }
+
+ ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
+ 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);
+ g_free(file);
+ return ret;
+}
+
+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;
+
+ opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
+ qemu_opts_absorb_qdict(opts, options, &local_err);
+ if (error_is_set(&local_err)) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ return -EINVAL;
+ }
+ ret = nfs_client_open(client, qemu_opt_get(opts, "filename"),
+ (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
+ errp);
+ if (ret < 0) {
+ return ret;
+ }
+ bs->total_sectors = ret;
+ return 0;
+}
+
+static int nfs_file_create(const char *url, QEMUOptionParameter *options,
+ Error **errp)
+{
+ int ret = 0;
+ int64_t total_size = 0;
+ NFSClient *client = g_malloc0(sizeof(NFSClient));
+
+ /* Read out options */
+ while (options && options->name) {
+ if (!strcmp(options->name, "size")) {
+ total_size = options->value.n;
+ }
+ options++;
+ }
+
+ ret = nfs_client_open(client, url, O_CREAT, errp);
+ if (ret < 0) {
+ goto out;
+ }
+ ret = nfs_ftruncate(client->context, client->fh, total_size);
+out:
+ nfs_client_close(client);
+ g_free(client);
+ return ret;
+}
+
+static int nfs_has_zero_init(BlockDriverState *bs)
+{
+ NFSClient *client = bs->opaque;
+ return client->has_zero_init;
+}
+
+static int64_t nfs_get_allocated_file_size(BlockDriverState *bs)
+{
+ NFSClient *client = bs->opaque;
+ NFSRPC task = {0};
+ struct stat st;
+
+ task.st = &st;
+ if (nfs_fstat_async(client->context, client->fh, nfs_co_generic_cb,
+ &task) != 0) {
+ return -ENOMEM;
+ }
+
+ while (!task.complete) {
+ nfs_set_events(client);
+ qemu_aio_wait();
+ }
+
+ return (task.status < 0 ? task.status : st.st_blocks * st.st_blksize);
+}
+
+static int nfs_file_truncate(BlockDriverState *bs, int64_t offset)
+{
+ NFSClient *client = bs->opaque;
+ return nfs_ftruncate(client->context, client->fh, offset);
+}
+
+static BlockDriver bdrv_nfs = {
+ .format_name = "nfs",
+ .protocol_name = "nfs",
+
+ .instance_size = sizeof(NFSClient),
+ .bdrv_needs_filename = true,
+ .bdrv_has_zero_init = nfs_has_zero_init,
+ .bdrv_get_allocated_file_size = nfs_get_allocated_file_size,
+ .bdrv_truncate = nfs_file_truncate,
+
+ .bdrv_file_open = nfs_file_open,
+ .bdrv_close = nfs_file_close,
+ .bdrv_create = nfs_file_create,
+
+ .bdrv_co_readv = nfs_co_readv,
+ .bdrv_co_writev = nfs_co_writev,
+ .bdrv_co_flush_to_disk = nfs_co_flush,
+};
+
+static void nfs_block_init(void)
+{
+ bdrv_register(&bdrv_nfs);
+}
+
+block_init(nfs_block_init);
diff --git a/configure b/configure
index b472694..9ca0c63 100755
--- a/configure
+++ b/configure
@@ -251,6 +251,7 @@ vss_win32_sdk=""
win_sdk="no"
want_tools="yes"
libiscsi=""
+libnfs=""
coroutine=""
coroutine_pool=""
seccomp=""
@@ -840,6 +841,10 @@ for opt do
;;
--enable-libiscsi) libiscsi="yes"
;;
+ --disable-libnfs) libnfs="no"
+ ;;
+ --enable-libnfs) libnfs="yes"
+ ;;
--enable-profiler) profiler="yes"
;;
--disable-cocoa) cocoa="no"
@@ -1229,6 +1234,8 @@ Advanced options (experts only):
--enable-rbd enable building the rados block device (rbd)
--disable-libiscsi disable iscsi support
--enable-libiscsi enable iscsi support
+ --disable-libnfs disable nfs support
+ --enable-libnfs enable nfs support
--disable-smartcard-nss disable smartcard nss support
--enable-smartcard-nss enable smartcard nss support
--disable-libusb disable libusb (for usb passthrough)
@@ -3600,6 +3607,20 @@ elif test "$debug" = "no" ; then
CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
fi
+##########################################
+# Do we have libnfs
+if test "$libnfs" != "no" ; then
+ if $pkg_config --atleast-version=1.8.91 libnfs; then
+ libnfs="yes"
+ libnfs_libs=$($pkg_config --libs libnfs)
+ LIBS="$LIBS $libnfs_libs"
+ else
+ if test "$libnfs" = "yes" ; then
+ feature_not_found "libnfs"
+ fi
+ libnfs="no"
+ fi
+fi
# Disable zero malloc errors for official releases unless explicitly told to
# enable/disable
@@ -3829,6 +3850,7 @@ echo "libiscsi support $libiscsi (1.4.0)"
else
echo "libiscsi support $libiscsi"
fi
+echo "libnfs support $libnfs"
echo "build guest agent $guest_agent"
echo "QGA VSS support $guest_agent_with_vss"
echo "seccomp support $seccomp"
@@ -4165,6 +4187,10 @@ if test "$libiscsi" = "yes" ; then
fi
fi
+if test "$libnfs" = "yes" ; then
+ echo "CONFIG_LIBNFS=y" >> $config_host_mak
+fi
+
if test "$seccomp" = "yes"; then
echo "CONFIG_SECCOMP=y" >> $config_host_mak
fi
diff --git a/qapi-schema.json b/qapi-schema.json
index 05ced9d..7cfb5e5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4371,6 +4371,7 @@
# TODO gluster: Wait for structured options
# TODO iscsi: Wait for structured options
# TODO nbd: Should take InetSocketAddress for 'host'?
+# TODO nfs: Wait for structured options
# TODO rbd: Wait for structured options
# TODO sheepdog: Wait for structured options
# TODO ssh: Should take InetSocketAddress for 'host'?
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCHv7 2/5] qemu-iotests: change _supported_proto to file for various tests
2014-01-29 8:50 [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 1/5] " Peter Lieven
@ 2014-01-29 8:50 ` Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 3/5] qemu-iotests: enable support for NFS protocol Peter Lieven
` (3 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Peter Lieven @ 2014-01-29 8:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, ronniesahlberg, Jeff Cody, Peter Lieven,
Max Reitz, owasserm, Federico Simoncelli, Stefan Hajnoczi,
Wenchao Xia
all these tests do anything of the following and thus fail with any
protocol other than file:
- the tests use rm, cp or mv shell commands which only work on file
- the tests use qcow2.py
- the images construct new filenames (e.g. backing file names) and
the logic is broken for anything else than file
Signed-off-by: Peter Lieven <pl@kamp.de>
---
tests/qemu-iotests/013 | 2 +-
tests/qemu-iotests/014 | 2 +-
tests/qemu-iotests/018 | 2 +-
tests/qemu-iotests/019 | 2 +-
tests/qemu-iotests/023 | 2 +-
tests/qemu-iotests/024 | 2 +-
tests/qemu-iotests/026 | 2 +-
tests/qemu-iotests/028 | 2 +-
tests/qemu-iotests/031 | 2 +-
tests/qemu-iotests/034 | 2 +-
tests/qemu-iotests/036 | 2 +-
tests/qemu-iotests/037 | 2 +-
tests/qemu-iotests/038 | 2 +-
tests/qemu-iotests/039 | 2 +-
tests/qemu-iotests/043 | 2 +-
tests/qemu-iotests/046 | 2 +-
tests/qemu-iotests/052 | 2 +-
tests/qemu-iotests/054 | 2 +-
tests/qemu-iotests/059 | 2 +-
tests/qemu-iotests/060 | 2 +-
tests/qemu-iotests/061 | 2 +-
tests/qemu-iotests/063 | 2 +-
tests/qemu-iotests/069 | 2 +-
23 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/tests/qemu-iotests/013 b/tests/qemu-iotests/013
index 389f4b8..ea3cab9 100755
--- a/tests/qemu-iotests/013
+++ b/tests/qemu-iotests/013
@@ -41,7 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# much of this could be generic for any format supporting compression.
_supported_fmt qcow qcow2
-_supported_proto generic
+_supported_proto file
_supported_os Linux
TEST_OFFSETS="0 4294967296"
diff --git a/tests/qemu-iotests/014 b/tests/qemu-iotests/014
index 0edeb4b..b23c2db 100755
--- a/tests/qemu-iotests/014
+++ b/tests/qemu-iotests/014
@@ -43,7 +43,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# much of this could be generic for any format supporting snapshots
_supported_fmt qcow2
-_supported_proto generic
+_supported_proto file
_supported_os Linux
TEST_OFFSETS="0 4294967296"
diff --git a/tests/qemu-iotests/018 b/tests/qemu-iotests/018
index 6f7f054..d8a7d43 100755
--- a/tests/qemu-iotests/018
+++ b/tests/qemu-iotests/018
@@ -41,7 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# Any format supporting backing files
_supported_fmt qcow qcow2 vmdk qed
-_supported_proto generic
+_supported_proto file
_supported_os Linux
_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
diff --git a/tests/qemu-iotests/019 b/tests/qemu-iotests/019
index b43e70f..e67445c 100755
--- a/tests/qemu-iotests/019
+++ b/tests/qemu-iotests/019
@@ -45,7 +45,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# Any format supporting backing files
_supported_fmt qcow qcow2 vmdk qed
-_supported_proto generic
+_supported_proto file
_supported_os Linux
_unsupported_imgopts "subformat=monolithicFlat" \
"subformat=twoGbMaxExtentFlat" \
diff --git a/tests/qemu-iotests/023 b/tests/qemu-iotests/023
index 090ed23..9ad06b9 100755
--- a/tests/qemu-iotests/023
+++ b/tests/qemu-iotests/023
@@ -41,7 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# much of this could be generic for any format supporting compression.
_supported_fmt qcow qcow2
-_supported_proto generic
+_supported_proto file
_supported_os Linux
TEST_OFFSETS="0 4294967296"
diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
index be974f0..9bf99e1 100755
--- a/tests/qemu-iotests/024
+++ b/tests/qemu-iotests/024
@@ -43,7 +43,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# Currently only qcow2 and qed support rebasing
_supported_fmt qcow2 qed
-_supported_proto generic
+_supported_proto file
_supported_os Linux
CLUSTER_SIZE=65536
diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index c9c5f83..df2884b 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -42,7 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# Currently only qcow2 supports rebasing
_supported_fmt qcow2
-_supported_proto generic
+_supported_proto file
_supported_os Linux
_default_cache_mode "writethrough"
_supported_cache_modes "writethrough" "none"
diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
index 93a9fa6..a99e4fa 100755
--- a/tests/qemu-iotests/028
+++ b/tests/qemu-iotests/028
@@ -45,7 +45,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# Any format supporting backing files except vmdk and qcow which do not support
# smaller backing files.
_supported_fmt qcow2 qed
-_supported_proto generic
+_supported_proto file
_supported_os Linux
# Choose a size that is not necessarily a cluster size multiple for image
diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
index c9070b0..1d920ea 100755
--- a/tests/qemu-iotests/031
+++ b/tests/qemu-iotests/031
@@ -41,7 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# This tests qcow2-specific low-level functionality
_supported_fmt qcow2
-_supported_proto generic
+_supported_proto file
_supported_os Linux
CLUSTER_SIZE=65536
diff --git a/tests/qemu-iotests/034 b/tests/qemu-iotests/034
index 7349789..69c7858 100755
--- a/tests/qemu-iotests/034
+++ b/tests/qemu-iotests/034
@@ -39,7 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.filter
_supported_fmt qcow qcow2 vmdk qed
-_supported_proto generic
+_supported_proto file
_supported_os Linux
_unsupported_imgopts "subformat=monolithicFlat" \
"subformat=twoGbMaxExtentFlat" \
diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
index e049a64..03b6aa9 100755
--- a/tests/qemu-iotests/036
+++ b/tests/qemu-iotests/036
@@ -44,7 +44,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# This tests qcow2-specific low-level functionality
_supported_fmt qcow2
-_supported_proto generic
+_supported_proto file
_supported_os Linux
# Only qcow2v3 and later supports feature bits
diff --git a/tests/qemu-iotests/037 b/tests/qemu-iotests/037
index e444349..9171d8c 100755
--- a/tests/qemu-iotests/037
+++ b/tests/qemu-iotests/037
@@ -39,7 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.filter
_supported_fmt qcow qcow2 vmdk qed
-_supported_proto generic
+_supported_proto file
_supported_os Linux
_unsupported_imgopts "subformat=monolithicFlat" \
"subformat=twoGbMaxExtentFlat" \
diff --git a/tests/qemu-iotests/038 b/tests/qemu-iotests/038
index 7bb7906..cfaf00a 100755
--- a/tests/qemu-iotests/038
+++ b/tests/qemu-iotests/038
@@ -39,7 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.filter
_supported_fmt qcow2 qed
-_supported_proto generic
+_supported_proto file
_supported_os Linux
CLUSTER_SIZE=2M
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 6abf472..9b355c0 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -42,7 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.filter
_supported_fmt qcow2
-_supported_proto generic
+_supported_proto file
_supported_os Linux
_default_cache_mode "writethrough"
_supported_cache_modes "writethrough"
diff --git a/tests/qemu-iotests/043 b/tests/qemu-iotests/043
index d7f1231..b316b97 100755
--- a/tests/qemu-iotests/043
+++ b/tests/qemu-iotests/043
@@ -41,7 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# Any format supporting backing files
_supported_fmt qcow2 qed
-_supported_proto generic
+_supported_proto file
_supported_os Linux
diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
index 3f17ceb..2d44bbb 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -39,7 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.filter
_supported_fmt qcow2
-_supported_proto generic
+_supported_proto file
_supported_os Linux
CLUSTER_SIZE=64k
diff --git a/tests/qemu-iotests/052 b/tests/qemu-iotests/052
index 4d4e411..6bdae92 100755
--- a/tests/qemu-iotests/052
+++ b/tests/qemu-iotests/052
@@ -39,7 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.filter
_supported_fmt generic
-_supported_proto generic
+_supported_proto file
_supported_os Linux
_default_cache_mode "writethrough"
_supported_cache_modes "writethrough"
diff --git a/tests/qemu-iotests/054 b/tests/qemu-iotests/054
index 5a0d1b1..c8b7082 100755
--- a/tests/qemu-iotests/054
+++ b/tests/qemu-iotests/054
@@ -39,7 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.filter
_supported_fmt qcow2
-_supported_proto generic
+_supported_proto file
_supported_os Linux
echo
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 2d604d3..ca5aa16 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# This tests vmdk-specific low-level functionality
_supported_fmt vmdk
-_supported_proto generic
+_supported_proto file
_supported_os Linux
_unsupported_imgopts "subformat=monolithicFlat" \
"subformat=twoGbMaxExtentFlat" \
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index bbb1909..af8ed9f 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# This tests qocw2-specific low-level functionality
_supported_fmt qcow2
-_supported_proto generic
+_supported_proto file
_supported_os Linux
rt_offset=65536 # 0x10000 (XXX: just an assumption)
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index e42f9bd..d3a6b38 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
# This tests qocw2-specific low-level functionality
_supported_fmt qcow2
-_supported_proto generic
+_supported_proto file
_supported_os Linux
echo
diff --git a/tests/qemu-iotests/063 b/tests/qemu-iotests/063
index 77503a2..a47493a 100755
--- a/tests/qemu-iotests/063
+++ b/tests/qemu-iotests/063
@@ -42,7 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.pattern
_supported_fmt qcow qcow2 vmdk qed raw
-_supported_proto generic
+_supported_proto file
_supported_os Linux
_unsupported_imgopts "subformat=monolithicFlat" \
"subformat=twoGbMaxExtentFlat" \
diff --git a/tests/qemu-iotests/069 b/tests/qemu-iotests/069
index 50347d9..e661598 100755
--- a/tests/qemu-iotests/069
+++ b/tests/qemu-iotests/069
@@ -39,7 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.filter
_supported_fmt cow qed qcow qcow2 vmdk
-_supported_proto generic
+_supported_proto file
_supported_os Linux
_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCHv7 3/5] qemu-iotests: enable support for NFS protocol
2014-01-29 8:50 [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 1/5] " Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 2/5] qemu-iotests: change _supported_proto to file for various tests Peter Lieven
@ 2014-01-29 8:50 ` Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 4/5] qemu-iotests: enable test 016 and 025 to work with " Peter Lieven
` (2 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Peter Lieven @ 2014-01-29 8:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, ronniesahlberg, Jeff Cody, Peter Lieven,
Max Reitz, owasserm, Federico Simoncelli, Stefan Hajnoczi,
Wenchao Xia
Signed-off-by: Peter Lieven <pl@kamp.de>
---
tests/qemu-iotests/common | 22 +++++++++++++++++++---
tests/qemu-iotests/common.rc | 3 +++
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index 8b4e22c..5795358 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -144,10 +144,12 @@ check options
-vpc test vpc
-vhdx test vhdx
-vmdk test vmdk
+ -file test file (default)
-rbd test rbd
-sheepdog test sheepdog
-nbd test nbd
-ssh test ssh
+ -nfs test nfs
-xdiff graphical mode diff
-nocache use O_DIRECT on backing file
-misalign misalign memory allocations
@@ -211,22 +213,36 @@ testlist options
xpand=false
;;
+ -file)
+ IMGPROTO=file
+ xpand=false
+ ;;
+
-rbd)
IMGPROTO=rbd
xpand=false
;;
+
-sheepdog)
IMGPROTO=sheepdog
xpand=false
;;
+
-nbd)
IMGPROTO=nbd
xpand=false
;;
+
-ssh)
IMGPROTO=ssh
xpand=false
;;
+
+ -nfs)
+ IMGPROTO=nfs
+ xpand=false
+ ;;
+
-nocache)
CACHEMODE="none"
CACHEMODE_IS_DEFAULT=false
@@ -238,10 +254,10 @@ testlist options
xpand=false
;;
- -valgrind)
- valgrind=true
+ -valgrind)
+ valgrind=true
xpand=false
- ;;
+ ;;
-g) # -g group ... pick from group file
group=true
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 0f68156..71e9a74 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -61,6 +61,9 @@ elif [ "$IMGPROTO" = "nbd" ]; then
elif [ "$IMGPROTO" = "ssh" ]; then
TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
TEST_IMG="ssh://127.0.0.1$TEST_IMG_FILE"
+elif [ "$IMGPROTO" = "nfs" ]; then
+ TEST_DIR="nfs://127.0.0.1/$TEST_DIR"
+ TEST_IMG=$TEST_DIR/t.$IMGFMT
else
TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
fi
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCHv7 4/5] qemu-iotests: enable test 016 and 025 to work with NFS protocol
2014-01-29 8:50 [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Peter Lieven
` (2 preceding siblings ...)
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 3/5] qemu-iotests: enable support for NFS protocol Peter Lieven
@ 2014-01-29 8:50 ` Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 5/5] qemu-iotests: blacklist test 020 for " Peter Lieven
2014-01-29 13:59 ` [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Stefan Hajnoczi
5 siblings, 0 replies; 23+ messages in thread
From: Peter Lieven @ 2014-01-29 8:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, ronniesahlberg, Jeff Cody, Peter Lieven,
Max Reitz, owasserm, Federico Simoncelli, Stefan Hajnoczi,
Wenchao Xia
Signed-off-by: Peter Lieven <pl@kamp.de>
---
tests/qemu-iotests/016 | 2 +-
tests/qemu-iotests/025 | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/016 b/tests/qemu-iotests/016
index b87a32b..7ea9e94 100755
--- a/tests/qemu-iotests/016
+++ b/tests/qemu-iotests/016
@@ -39,7 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.filter
_supported_fmt raw
-_supported_proto file sheepdog
+_supported_proto file sheepdog nfs
_supported_os Linux
diff --git a/tests/qemu-iotests/025 b/tests/qemu-iotests/025
index 9426c93..a5f45b4 100755
--- a/tests/qemu-iotests/025
+++ b/tests/qemu-iotests/025
@@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.pattern
_supported_fmt raw qcow2 qed
-_supported_proto file sheepdog rbd
+_supported_proto file sheepdog rbd nfs
_supported_os Linux
echo "=== Creating image"
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCHv7 5/5] qemu-iotests: blacklist test 020 for NFS protocol
2014-01-29 8:50 [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Peter Lieven
` (3 preceding siblings ...)
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 4/5] qemu-iotests: enable test 016 and 025 to work with " Peter Lieven
@ 2014-01-29 8:50 ` Peter Lieven
2014-01-29 13:59 ` [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Stefan Hajnoczi
5 siblings, 0 replies; 23+ messages in thread
From: Peter Lieven @ 2014-01-29 8:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, ronniesahlberg, Jeff Cody, Peter Lieven,
Max Reitz, owasserm, Federico Simoncelli, Stefan Hajnoczi,
Wenchao Xia
reopening is currently not supported.
Signed-off-by: Peter Lieven <pl@kamp.de>
---
tests/qemu-iotests/020 | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/tests/qemu-iotests/020 b/tests/qemu-iotests/020
index 73a0429..2f258dc 100755
--- a/tests/qemu-iotests/020
+++ b/tests/qemu-iotests/020
@@ -49,6 +49,11 @@ _unsupported_imgopts "subformat=monolithicFlat" \
"subformat=twoGbMaxExtentFlat" \
"subformat=twoGbMaxExtentSparse"
+# NFS does not support bdrv_reopen_prepare thus qemu-img commit fails.
+if [ "$IMGPROTO" = "nfs" ]; then
+ _notrun "image protocol $IMGPROTO does not support bdrv_commit"
+fi
+
TEST_OFFSETS="0 4294967296"
_make_test_img 6G
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS
2014-01-29 8:50 [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Peter Lieven
` (4 preceding siblings ...)
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 5/5] qemu-iotests: blacklist test 020 for " Peter Lieven
@ 2014-01-29 13:59 ` Stefan Hajnoczi
2014-01-29 14:13 ` Peter Lieven
2014-01-29 14:22 ` Peter Lieven
5 siblings, 2 replies; 23+ messages in thread
From: Stefan Hajnoczi @ 2014-01-29 13:59 UTC (permalink / raw)
To: Peter Lieven
Cc: Kevin Wolf, Fam Zheng, Jeff Cody, qemu-devel, Max Reitz,
owasserm, Federico Simoncelli, ronniesahlberg, Wenchao Xia
On Wed, Jan 29, 2014 at 09:50:20AM +0100, Peter Lieven wrote:
> This adds v6 of the NFS protocol driver + qemu-iotest adjustments.
>
> v6->v7:
> - removed unsused and partially non-existing libnfs header file includes [Stefan]
> - adjusted usage of qemu_opts_create_nofail [Stefan]
> - removed patch to fix output of qemu-iotest 067
> - only blacklist qemu-iotest 020 for NFS do not change proto from generic to file [Fam]
It compiles now but I can't create an image on my /home NFS export:
$ ./qemu-img create -f qcow2 'nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000' 8G
Formatting 'nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000', fmt=qcow2 size=8589934592 encryption=off cluster_size=65536 lazy_refcounts=off
qemu-img: nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000: Failed to create file: nfs_creat_async failed
I added printfs to the code, block/nfs.c is mounting /home/stefanha/qemu
and tries to create the file /test-nfs.img.
I'm using the Linux kernel NFS server. Unfortunately I don't have more
time to debug this but I'd like to test this feature successfully before
merging it into QEMU. Any ideas why libnfs refuses to create the file?
(It's failing in nfs_lookuppath_async() with "Path is empty".)
Ronnie: libnfs clobbers the nfs->rpc error string so you cannot see the
detailed error message:
In this case nfs_creat() clobbers the error with "Failed to create file:
nfs_creat_async failed". In fact the message was "Out of memory: failed
to start parsing the path components" from nfs_creat_async(), but that's
clobbering the true error too. It really should be "Path is empty" from
nfs_lookuppath_async().
It would be nice to preserve the full error string, otherwise the user
cannot make sense of the problem.
Stefan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS
2014-01-29 13:59 ` [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Stefan Hajnoczi
@ 2014-01-29 14:13 ` Peter Lieven
2014-01-29 14:22 ` Peter Lieven
1 sibling, 0 replies; 23+ messages in thread
From: Peter Lieven @ 2014-01-29 14:13 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Fam Zheng, Jeff Cody, qemu-devel, Max Reitz,
owasserm, Federico Simoncelli, ronniesahlberg, Wenchao Xia
On 29.01.2014 14:59, Stefan Hajnoczi wrote:
> On Wed, Jan 29, 2014 at 09:50:20AM +0100, Peter Lieven wrote:
>> This adds v6 of the NFS protocol driver + qemu-iotest adjustments.
>>
>> v6->v7:
>> - removed unsused and partially non-existing libnfs header file includes [Stefan]
>> - adjusted usage of qemu_opts_create_nofail [Stefan]
>> - removed patch to fix output of qemu-iotest 067
>> - only blacklist qemu-iotest 020 for NFS do not change proto from generic to file [Fam]
> It compiles now but I can't create an image on my /home NFS export:
>
> $ ./qemu-img create -f qcow2 'nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000' 8G
> Formatting 'nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000', fmt=qcow2 size=8589934592 encryption=off cluster_size=65536 lazy_refcounts=off
> qemu-img: nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000: Failed to create file: nfs_creat_async failed
>
> I added printfs to the code, block/nfs.c is mounting /home/stefanha/qemu
> and tries to create the file /test-nfs.img.
>
> I'm using the Linux kernel NFS server. Unfortunately I don't have more
> time to debug this but I'd like to test this feature successfully before
> merging it into QEMU. Any ideas why libnfs refuses to create the file?
> (It's failing in nfs_lookuppath_async() with "Path is empty".)
>
> Ronnie: libnfs clobbers the nfs->rpc error string so you cannot see the
> detailed error message:
>
> In this case nfs_creat() clobbers the error with "Failed to create file:
> nfs_creat_async failed". In fact the message was "Out of memory: failed
> to start parsing the path components" from nfs_creat_async(), but that's
> clobbering the true error too. It really should be "Path is empty" from
> nfs_lookuppath_async().
>
> It would be nice to preserve the full error string, otherwise the user
> cannot make sense of the problem.
>
> Stefan
I do not understand why your path is empty...
Running almost the same test:
~/git/qemu$ ./qemu-img create -f qcow2 'nfs://127.0.0.1/home/lieven/git/qemu/test-nfs.img?uid=1000&gid=1000' 8G
Formatting 'nfs://127.0.0.1/home/lieven/git/qemu/test-nfs.img?uid=1000&gid=1000', fmt=qcow2 size=8589934592 encryption=off cluster_size=65536 lazy_refcounts=off
qemu-img: nfs://127.0.0.1/home/lieven/git/qemu/test-nfs.img?uid=1000&gid=1000: Failed to mount nfs share: mount/mnt call failed with "RPC error: Mount failed with error MNT3ERR_ACCES(13) Permission denied(13)"
I have to run as root or specify 'insecure' option in /etc/exports
~/git/qemu$ sudo ./qemu-img create -f qcow2 'nfs://127.0.0.1/home/lieven/git/qemu/test-nfs.img?uid=1000&gid=1000' 8G
Formatting 'nfs://127.0.0.1/home/lieven/git/qemu/test-nfs.img?uid=1000&gid=1000', fmt=qcow2 size=8589934592 encryption=off cluster_size=65536 lazy_refcounts=off
The last succeeds without problems.
Peter
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS
2014-01-29 13:59 ` [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Stefan Hajnoczi
2014-01-29 14:13 ` Peter Lieven
@ 2014-01-29 14:22 ` Peter Lieven
2014-01-29 14:26 ` Peter Lieven
1 sibling, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2014-01-29 14:22 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Fam Zheng, Jeff Cody, qemu-devel, Max Reitz,
owasserm, Federico Simoncelli, ronniesahlberg, Wenchao Xia
On 29.01.2014 14:59, Stefan Hajnoczi wrote:
> On Wed, Jan 29, 2014 at 09:50:20AM +0100, Peter Lieven wrote:
>> This adds v6 of the NFS protocol driver + qemu-iotest adjustments.
>>
>> v6->v7:
>> - removed unsused and partially non-existing libnfs header file includes [Stefan]
>> - adjusted usage of qemu_opts_create_nofail [Stefan]
>> - removed patch to fix output of qemu-iotest 067
>> - only blacklist qemu-iotest 020 for NFS do not change proto from generic to file [Fam]
> It compiles now but I can't create an image on my /home NFS export:
>
> $ ./qemu-img create -f qcow2 'nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000' 8G
> Formatting 'nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000', fmt=qcow2 size=8589934592 encryption=off cluster_size=65536 lazy_refcounts=off
> qemu-img: nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000: Failed to create file: nfs_creat_async failed
>
> I added printfs to the code, block/nfs.c is mounting /home/stefanha/qemu
> and tries to create the file /test-nfs.img.
>
> I'm using the Linux kernel NFS server. Unfortunately I don't have more
> time to debug this but I'd like to test this feature successfully before
> merging it into QEMU. Any ideas why libnfs refuses to create the file?
> (It's failing in nfs_lookuppath_async() with "Path is empty".)
>
> Ronnie: libnfs clobbers the nfs->rpc error string so you cannot see the
> detailed error message:
>
> In this case nfs_creat() clobbers the error with "Failed to create file:
> nfs_creat_async failed". In fact the message was "Out of memory: failed
> to start parsing the path components" from nfs_creat_async(), but that's
> clobbering the true error too. It really should be "Path is empty" from
> nfs_lookuppath_async().
It seems the version in Ronnies repository got broken somewhere after
my commit d43a8953f5. Can you try with:
https://github.com/plieven/libnfs
Thanks,
Peter
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS
2014-01-29 14:22 ` Peter Lieven
@ 2014-01-29 14:26 ` Peter Lieven
0 siblings, 0 replies; 23+ messages in thread
From: Peter Lieven @ 2014-01-29 14:26 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Fam Zheng, Jeff Cody, qemu-devel, Max Reitz,
owasserm, Federico Simoncelli, ronniesahlberg, Wenchao Xia
On 29.01.2014 15:22, Peter Lieven wrote:
> On 29.01.2014 14:59, Stefan Hajnoczi wrote:
>> On Wed, Jan 29, 2014 at 09:50:20AM +0100, Peter Lieven wrote:
>>> This adds v6 of the NFS protocol driver + qemu-iotest adjustments.
>>>
>>> v6->v7:
>>> - removed unsused and partially non-existing libnfs header file includes [Stefan]
>>> - adjusted usage of qemu_opts_create_nofail [Stefan]
>>> - removed patch to fix output of qemu-iotest 067
>>> - only blacklist qemu-iotest 020 for NFS do not change proto from generic to file [Fam]
>> It compiles now but I can't create an image on my /home NFS export:
>>
>> $ ./qemu-img create -f qcow2 'nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000' 8G
>> Formatting 'nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000', fmt=qcow2 size=8589934592 encryption=off cluster_size=65536 lazy_refcounts=off
>> qemu-img: nfs://127.0.0.1/home/stefanha/qemu/test-nfs.img?uid=1000&gid=1000: Failed to create file: nfs_creat_async failed
>>
>> I added printfs to the code, block/nfs.c is mounting /home/stefanha/qemu
>> and tries to create the file /test-nfs.img.
>>
>> I'm using the Linux kernel NFS server. Unfortunately I don't have more
>> time to debug this but I'd like to test this feature successfully before
>> merging it into QEMU. Any ideas why libnfs refuses to create the file?
>> (It's failing in nfs_lookuppath_async() with "Path is empty".)
>>
>> Ronnie: libnfs clobbers the nfs->rpc error string so you cannot see the
>> detailed error message:
>>
>> In this case nfs_creat() clobbers the error with "Failed to create file:
>> nfs_creat_async failed". In fact the message was "Out of memory: failed
>> to start parsing the path components" from nfs_creat_async(), but that's
>> clobbering the true error too. It really should be "Path is empty" from
>> nfs_lookuppath_async().
> It seems the version in Ronnies repository got broken somewhere after
> my commit d43a8953f5. Can you try with:
>
> https://github.com/plieven/libnfs
Ronnie, commit fb6f3152 "Add support for chdir and getcwd" is bad.
Peter
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 1/5] " Peter Lieven
@ 2014-01-29 16:19 ` Benoît Canet
2014-01-29 16:38 ` Peter Lieven
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Benoît Canet @ 2014-01-29 16:19 UTC (permalink / raw)
To: Peter Lieven
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Jeff Cody, qemu-devel,
Max Reitz, owasserm, Federico Simoncelli, ronniesahlberg,
Wenchao Xia
Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
> This patch adds native support for accessing images on NFS
> shares without the requirement to actually mount the entire
> NFS share on the host.
>
> NFS Images can simply be specified by an url of the form:
> nfs://<host>/<export>/<filename>[?param=value[¶m2=value2[&...]]]
>
> For example:
> qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2
>
> You need LibNFS from Ronnie Sahlberg available at:
> git://github.com/sahlberg/libnfs.git
> for this to work.
>
> During configure it is automatically probed for libnfs and support
> is enabled on-the-fly. You can forbid or enforce libnfs support
> with --disable-libnfs or --enable-libnfs respectively.
>
> Due to NFS restrictions you might need to execute your binaries
> as root, allow them to open priviledged ports (<1024) or specify
> insecure option on the NFS server.
>
> For additional information on ROOT vs. non-ROOT operation and URL
> format + parameters see:
> https://raw.github.com/sahlberg/libnfs/master/README
>
> Supported by qemu are the uid, gid and tcp-syncnt URL parameters.
>
> LibNFS currently support NFS version 3 only.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> MAINTAINERS | 5 +
> block/Makefile.objs | 1 +
> block/nfs.c | 440 +++++++++++++++++++++++++++++++++++++++++++++++++++
> configure | 26 +++
> qapi-schema.json | 1 +
> 5 files changed, 473 insertions(+)
> create mode 100644 block/nfs.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fb53242..f8411f9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -936,6 +936,11 @@ M: Peter Lieven <pl@kamp.de>
> S: Supported
> F: block/iscsi.c
>
> +NFS
> +M: Peter Lieven <pl@kamp.de>
> +S: Maintained
> +F: block/nfs.c
> +
> SSH
> M: Richard W.M. Jones <rjones@redhat.com>
> S: Supported
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 4e8c91e..e254a21 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> ifeq ($(CONFIG_POSIX),y)
> block-obj-y += nbd.o nbd-client.o sheepdog.o
> block-obj-$(CONFIG_LIBISCSI) += iscsi.o
> +block-obj-$(CONFIG_LIBNFS) += nfs.o
> block-obj-$(CONFIG_CURL) += curl.o
> block-obj-$(CONFIG_RBD) += rbd.o
> block-obj-$(CONFIG_GLUSTERFS) += gluster.o
> diff --git a/block/nfs.c b/block/nfs.c
> new file mode 100644
> index 0000000..2bbf7a2
> --- /dev/null
> +++ b/block/nfs.c
> @@ -0,0 +1,440 @@
> +/*
> + * QEMU Block driver for native access to files on NFS shares
> + *
> + * Copyright (c) 2014 Peter Lieven <pl@kamp.de>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "config-host.h"
> +
> +#include <poll.h>
> +#include "qemu-common.h"
> +#include "qemu/config-file.h"
> +#include "qemu/error-report.h"
> +#include "block/block_int.h"
> +#include "trace.h"
> +#include "qemu/iov.h"
> +#include "qemu/uri.h"
> +#include "sysemu/sysemu.h"
> +#include <nfsc/libnfs.h>
> +
> +typedef struct NFSClient {
> + struct nfs_context *context;
> + struct nfsfh *fh;
> + int events;
> + bool has_zero_init;
> +} NFSClient;
> +
> +typedef struct NFSRPC {
> + int status;
> + int complete;
> + QEMUIOVector *iov;
> + struct stat *st;
> + Coroutine *co;
> + QEMUBH *bh;
> +} NFSRPC;
> +
> +static void nfs_process_read(void *arg);
> +static void nfs_process_write(void *arg);
> +
> +static void nfs_set_events(NFSClient *client)
> +{
> + int ev = nfs_which_events(client->context);
> + if (ev != client->events) {
> + qemu_aio_set_fd_handler(nfs_get_fd(client->context),
> + (ev & POLLIN) ? nfs_process_read : NULL,
> + (ev & POLLOUT) ? nfs_process_write : NULL,
> + client);
> +
> + }
> + client->events = ev;
> +}
> +
> +static void nfs_process_read(void *arg)
> +{
> + NFSClient *client = arg;
> + nfs_service(client->context, POLLIN);
> + nfs_set_events(client);
> +}
> +
> +static void nfs_process_write(void *arg)
> +{
> + NFSClient *client = arg;
> + nfs_service(client->context, POLLOUT);
> + nfs_set_events(client);
> +}
> +
> +static void nfs_co_init_task(NFSClient *client, NFSRPC *task)
> +{
> + *task = (NFSRPC) {
> + .co = qemu_coroutine_self(),
> + };
> +}
> +
> +static void nfs_co_generic_bh_cb(void *opaque)
> +{
> + NFSRPC *task = opaque;
> + qemu_bh_delete(task->bh);
> + qemu_coroutine_enter(task->co, NULL);
> +}
> +
> +static void
> +nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
> + void *private_data)
> +{
> + NFSRPC *task = private_data;
> + task->complete = 1;
> + task->status = status;
> + if (task->status > 0 && task->iov) {
> + if (task->status <= task->iov->size) {
It feel very odd to compare something named status with something named size.
> + qemu_iovec_from_buf(task->iov, 0, data, task->status);
> + } else {
> + task->status = -EIO;
> + }
> + }
> + if (task->status == 0 && task->st) {
> + memcpy(task->st, data, sizeof(struct stat));
> + }
> + if (task->co) {
> + task->bh = qemu_bh_new(nfs_co_generic_bh_cb, task);
> + qemu_bh_schedule(task->bh);
> + }
> +}
> +
> +static int coroutine_fn nfs_co_readv(BlockDriverState *bs,
> + int64_t sector_num, int nb_sectors,
> + QEMUIOVector *iov)
> +{
> + NFSClient *client = bs->opaque;
> + NFSRPC task;
> +
> + nfs_co_init_task(client, &task);
> + task.iov = iov;
> +
> + if (nfs_pread_async(client->context, client->fh,
> + sector_num * BDRV_SECTOR_SIZE,
> + nb_sectors * BDRV_SECTOR_SIZE,
> + nfs_co_generic_cb, &task) != 0) {
> + return -ENOMEM;
> + }
> +
> + while (!task.complete) {
> + nfs_set_events(client);
> + qemu_coroutine_yield();
> + }
> +
> + if (task.status < 0) {
> + return task.status;
> + }
> +
> + /* zero pad short reads */
> + if (task.status < iov->size) {
> + qemu_iovec_memset(iov, task.status, 0, iov->size - task.status);
> + }
> +
> + return 0;
> +}
> +
> +static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
> + int64_t sector_num, int nb_sectors,
> + QEMUIOVector *iov)
> +{
> + NFSClient *client = bs->opaque;
> + NFSRPC task;
> + char *buf = NULL;
> +
> + nfs_co_init_task(client, &task);
> +
> + buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
> + qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
> +
> + if (nfs_pwrite_async(client->context, client->fh,
> + sector_num * BDRV_SECTOR_SIZE,
> + nb_sectors * BDRV_SECTOR_SIZE,
> + buf, nfs_co_generic_cb, &task) != 0) {
> + g_free(buf);
> + return -ENOMEM;
> + }
> +
> + while (!task.complete) {
> + nfs_set_events(client);
> + qemu_coroutine_yield();
> + }
> +
> + g_free(buf);
> +
> + if (task.status != nb_sectors * BDRV_SECTOR_SIZE) {
> + return task.status < 0 ? task.status : -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
> +{
> + NFSClient *client = bs->opaque;
> + NFSRPC task;
> +
> + nfs_co_init_task(client, &task);
> +
> + if (nfs_fsync_async(client->context, client->fh, nfs_co_generic_cb,
> + &task) != 0) {
> + return -ENOMEM;
> + }
> +
> + while (!task.complete) {
> + nfs_set_events(client);
> + qemu_coroutine_yield();
> + }
> +
> + return task.status;
> +}
> +
> +/* TODO Convert to fine grained options */
> +static QemuOptsList runtime_opts = {
> + .name = "nfs",
> + .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> + .desc = {
> + {
> + .name = "filename",
> + .type = QEMU_OPT_STRING,
> + .help = "URL to the NFS file",
> + },
> + { /* end of list */ }
> + },
> +};
> +
> +static void nfs_client_close(NFSClient *client)
> +{
> + if (client->context) {
> + if (client->fh) {
> + nfs_close(client->context, client->fh);
> + }
> + qemu_aio_set_fd_handler(nfs_get_fd(client->context), NULL, NULL, NULL);
> + nfs_destroy_context(client->context);
> + }
> + memset(client, 0, sizeof(NFSClient));
> +}
> +
> +static void nfs_file_close(BlockDriverState *bs)
> +{
> + NFSClient *client = bs->opaque;
> + nfs_client_close(client);
> +}
> +
> +static int64_t nfs_.cilclient_open(NFSClient *client, const char *filename,
> + int flags, Error **errp)
> +{
> + int ret = -EINVAL, i;
> + 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");
> + goto fail;
Should this be goto out since we don't have done nfs_init_context yet ?
> + }
> + strp = strrchr(uri->path, '/');
> + if (strp == NULL) {
> + error_setg(errp, "Invalid URL specified");
> + goto fail;
goto out ?
> + }
> + file = g_strdup(strp);
> + *strp = 0;
> +
> + client->context = nfs_init_context();
> + if (client->context == NULL) {
> + error_setg(errp, "Failed to init NFS context");
> + goto fail;
> + }
> +
> + qp = query_params_parse(uri->query);
> + 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 fail;
> + }
> + if (!strncmp(qp->p[i].name, "uid", 3)) {
> + nfs_set_uid(client->context, atoi(qp->p[i].value));
> + } else if (!strncmp(qp->p[i].name, "gid", 3)) {
> + nfs_set_gid(client->context, atoi(qp->p[i].value));
> + } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
> + nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
> + } else {
> + error_setg(errp, "Unknown NFS parameter name: %s",
> + qp->p[i].name);
> + goto fail;
> + }
> + }
> +
> + ret = nfs_mount(client->context, uri->server, uri->path);
> + if (ret < 0) {
> + error_setg(errp, "Failed to mount nfs share: %s",
> + nfs_get_error(client->context));
> + goto fail;
> + }
> +
> + if (flags & O_CREAT) {
> + ret = nfs_creat(client->context, file, 0600, &client->fh);
> + if (ret < 0) {
> + error_setg(errp, "Failed to create file: %s",
> + nfs_get_error(client->context));
> + goto fail;
> + }
> + } else {
> + ret = nfs_open(client->context, file, flags, &client->fh);
> + if (ret < 0) {
> + error_setg(errp, "Failed to open file : %s",
> + nfs_get_error(client->context));
> + goto fail;
> + }
> + }
> +
> + ret = nfs_fstat(client->context, client->fh, &st);
> + if (ret < 0) {
> + error_setg(errp, "Failed to fstat file: %s",
> + nfs_get_error(client->context));
> + goto fail;
> + }
> +
> + ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
> + 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);
> + g_free(file);
> + return ret;
> +}
> +
> +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;
> +
> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> + qemu_opts_absorb_qdict(opts, options, &local_err);
> + if (error_is_set(&local_err)) {
> + qerror_report_err(local_err);
I have seen more usage of error_propagate(errp, local_err); in QEMU code.
Maybe I am missing the point.
> + error_free(local_err);
> + return -EINVAL;
> + }
> + ret = nfs_client_open(client, qemu_opt_get(opts, "filename"),
> + (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
> + errp);
> + if (ret < 0) {
> + return ret;
> + }
> + bs->total_sectors = ret;
> + return 0;
> +}
> +
> +static int nfs_file_create(const char *url, QEMUOptionParameter *options,
> + Error **errp)
> +{
> + int ret = 0;
> + int64_t total_size = 0;
> + NFSClient *client = g_malloc0(sizeof(NFSClient));
> +
> + /* Read out options */
> + while (options && options->name) {
> + if (!strcmp(options->name, "size")) {
> + total_size = options->value.n;
> + }
> + options++;
> + }
> +
> + ret = nfs_client_open(client, url, O_CREAT, errp);
> + if (ret < 0) {
> + goto out;
> + }
> + ret = nfs_ftruncate(client->context, client->fh, total_size);
> +out:
Should the out: label be after nfs_client_close(client); since the code will
jump to it on nfs_client_open failure ?
> + nfs_client_close(client);
> + g_free(client);
> + return ret;
> +}
> +
> +static int nfs_has_zero_init(BlockDriverState *bs)
> +{
> + NFSClient *client = bs->opaque;
> + return client->has_zero_init;
> +}
> +
> +static int64_t nfs_get_allocated_file_size(BlockDriverState *bs)
> +{
> + NFSClient *client = bs->opaque;
> + NFSRPC task = {0};
> + struct stat st;
> +
> + task.st = &st;
> + if (nfs_fstat_async(client->context, client->fh, nfs_co_generic_cb,
> + &task) != 0) {
> + return -ENOMEM;
> + }
> +
> + while (!task.complete) {
> + nfs_set_events(client);
> + qemu_aio_wait();
> + }
> +
> + return (task.status < 0 ? task.status : st.st_blocks * st.st_blksize);
> +}
> +
> +static int nfs_file_truncate(BlockDriverState *bs, int64_t offset)
> +{
> + NFSClient *client = bs->opaque;
> + return nfs_ftruncate(client->context, client->fh, offset);
> +}
> +
> +static BlockDriver bdrv_nfs = {
> + .format_name = "nfs",
> + .protocol_name = "nfs",
> +
> + .instance_size = sizeof(NFSClient),
> + .bdrv_needs_filename = true,
> + .bdrv_has_zero_init = nfs_has_zero_init,
> + .bdrv_get_allocated_file_size = nfs_get_allocated_file_size,
> + .bdrv_truncate = nfs_file_truncate,
> +
> + .bdrv_file_open = nfs_file_open,
> + .bdrv_close = nfs_file_close,
> + .bdrv_create = nfs_file_create,
> +
> + .bdrv_co_readv = nfs_co_readv,
> + .bdrv_co_writev = nfs_co_writev,
> + .bdrv_co_flush_to_disk = nfs_co_flush,
> +};
> +
> +static void nfs_block_init(void)
> +{
> + bdrv_register(&bdrv_nfs);
> +}
> +
> +block_init(nfs_block_init);
> diff --git a/configure b/configure
> index b472694..9ca0c63 100755
> --- a/configure
> +++ b/configure
> @@ -251,6 +251,7 @@ vss_win32_sdk=""
> win_sdk="no"
> want_tools="yes"
> libiscsi=""
> +libnfs=""
> coroutine=""
> coroutine_pool=""
> seccomp=""
> @@ -840,6 +841,10 @@ for opt do
> ;;
> --enable-libiscsi) libiscsi="yes"
> ;;
> + --disable-libnfs) libnfs="no"
> + ;;
> + --enable-libnfs) libnfs="yes"
> + ;;
> --enable-profiler) profiler="yes"
> ;;
> --disable-cocoa) cocoa="no"
> @@ -1229,6 +1234,8 @@ Advanced options (experts only):
> --enable-rbd enable building the rados block device (rbd)
> --disable-libiscsi disable iscsi support
> --enable-libiscsi enable iscsi support
> + --disable-libnfs disable nfs support
> + --enable-libnfs enable nfs support
> --disable-smartcard-nss disable smartcard nss support
> --enable-smartcard-nss enable smartcard nss support
> --disable-libusb disable libusb (for usb passthrough)
> @@ -3600,6 +3607,20 @@ elif test "$debug" = "no" ; then
> CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
> fi
>
> +##########################################
> +# Do we have libnfs
> +if test "$libnfs" != "no" ; then
> + if $pkg_config --atleast-version=1.8.91 libnfs; then
> + libnfs="yes"
> + libnfs_libs=$($pkg_config --libs libnfs)
> + LIBS="$LIBS $libnfs_libs"
> + else
> + if test "$libnfs" = "yes" ; then
> + feature_not_found "libnfs"
> + fi
> + libnfs="no"
> + fi
> +fi
>
> # Disable zero malloc errors for official releases unless explicitly told to
> # enable/disable
> @@ -3829,6 +3850,7 @@ echo "libiscsi support $libiscsi (1.4.0)"
> else
> echo "libiscsi support $libiscsi"
> fi
> +echo "libnfs support $libnfs"
> echo "build guest agent $guest_agent"
> echo "QGA VSS support $guest_agent_with_vss"
> echo "seccomp support $seccomp"
> @@ -4165,6 +4187,10 @@ if test "$libiscsi" = "yes" ; then
> fi
> fi
>
> +if test "$libnfs" = "yes" ; then
> + echo "CONFIG_LIBNFS=y" >> $config_host_mak
> +fi
> +
> if test "$seccomp" = "yes"; then
> echo "CONFIG_SECCOMP=y" >> $config_host_mak
> fi
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 05ced9d..7cfb5e5 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4371,6 +4371,7 @@
> # TODO gluster: Wait for structured options
> # TODO iscsi: Wait for structured options
> # TODO nbd: Should take InetSocketAddress for 'host'?
> +# TODO nfs: Wait for structured options
> # TODO rbd: Wait for structured options
> # TODO sheepdog: Wait for structured options
> # TODO ssh: Should take InetSocketAddress for 'host'?
> --
> 1.7.9.5
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-29 16:19 ` Benoît Canet
@ 2014-01-29 16:38 ` Peter Lieven
2014-01-30 9:05 ` Stefan Hajnoczi
2014-01-30 14:22 ` Stefan Hajnoczi
2014-01-31 11:36 ` Peter Lieven
2 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2014-01-29 16:38 UTC (permalink / raw)
To: Benoît Canet
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Jeff Cody, qemu-devel,
Max Reitz, owasserm, Federico Simoncelli, ronniesahlberg,
Wenchao Xia
On 29.01.2014 17:19, Benoît Canet wrote:
> Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
>> This patch adds native support for accessing images on NFS
>> shares without the requirement to actually mount the entire
>> NFS share on the host.
>>
>> NFS Images can simply be specified by an url of the form:
>> nfs://<host>/<export>/<filename>[?param=value[¶m2=value2[&...]]]
>>
>> For example:
>> qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2
>>
>> You need LibNFS from Ronnie Sahlberg available at:
>> git://github.com/sahlberg/libnfs.git
>> for this to work.
>>
>> During configure it is automatically probed for libnfs and support
>> is enabled on-the-fly. You can forbid or enforce libnfs support
>> with --disable-libnfs or --enable-libnfs respectively.
>>
>> Due to NFS restrictions you might need to execute your binaries
>> as root, allow them to open priviledged ports (<1024) or specify
>> insecure option on the NFS server.
>>
>> For additional information on ROOT vs. non-ROOT operation and URL
>> format + parameters see:
>> https://raw.github.com/sahlberg/libnfs/master/README
>>
>> Supported by qemu are the uid, gid and tcp-syncnt URL parameters.
>>
>> LibNFS currently support NFS version 3 only.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> MAINTAINERS | 5 +
>> block/Makefile.objs | 1 +
>> block/nfs.c | 440 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> configure | 26 +++
>> qapi-schema.json | 1 +
>> 5 files changed, 473 insertions(+)
>> create mode 100644 block/nfs.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index fb53242..f8411f9 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -936,6 +936,11 @@ M: Peter Lieven <pl@kamp.de>
>> S: Supported
>> F: block/iscsi.c
>>
>> +NFS
>> +M: Peter Lieven <pl@kamp.de>
>> +S: Maintained
>> +F: block/nfs.c
>> +
>> SSH
>> M: Richard W.M. Jones <rjones@redhat.com>
>> S: Supported
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index 4e8c91e..e254a21 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>> ifeq ($(CONFIG_POSIX),y)
>> block-obj-y += nbd.o nbd-client.o sheepdog.o
>> block-obj-$(CONFIG_LIBISCSI) += iscsi.o
>> +block-obj-$(CONFIG_LIBNFS) += nfs.o
>> block-obj-$(CONFIG_CURL) += curl.o
>> block-obj-$(CONFIG_RBD) += rbd.o
>> block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>> diff --git a/block/nfs.c b/block/nfs.c
>> new file mode 100644
>> index 0000000..2bbf7a2
>> --- /dev/null
>> +++ b/block/nfs.c
>> @@ -0,0 +1,440 @@
>> +/*
>> + * QEMU Block driver for native access to files on NFS shares
>> + *
>> + * Copyright (c) 2014 Peter Lieven <pl@kamp.de>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "config-host.h"
>> +
>> +#include <poll.h>
>> +#include "qemu-common.h"
>> +#include "qemu/config-file.h"
>> +#include "qemu/error-report.h"
>> +#include "block/block_int.h"
>> +#include "trace.h"
>> +#include "qemu/iov.h"
>> +#include "qemu/uri.h"
>> +#include "sysemu/sysemu.h"
>> +#include <nfsc/libnfs.h>
>> +
>> +typedef struct NFSClient {
>> + struct nfs_context *context;
>> + struct nfsfh *fh;
>> + int events;
>> + bool has_zero_init;
>> +} NFSClient;
>> +
>> +typedef struct NFSRPC {
>> + int status;
>> + int complete;
>> + QEMUIOVector *iov;
>> + struct stat *st;
>> + Coroutine *co;
>> + QEMUBH *bh;
>> +} NFSRPC;
>> +
>> +static void nfs_process_read(void *arg);
>> +static void nfs_process_write(void *arg);
>> +
>> +static void nfs_set_events(NFSClient *client)
>> +{
>> + int ev = nfs_which_events(client->context);
>> + if (ev != client->events) {
>> + qemu_aio_set_fd_handler(nfs_get_fd(client->context),
>> + (ev & POLLIN) ? nfs_process_read : NULL,
>> + (ev & POLLOUT) ? nfs_process_write : NULL,
>> + client);
>> +
>> + }
>> + client->events = ev;
>> +}
>> +
>> +static void nfs_process_read(void *arg)
>> +{
>> + NFSClient *client = arg;
>> + nfs_service(client->context, POLLIN);
>> + nfs_set_events(client);
>> +}
>> +
>> +static void nfs_process_write(void *arg)
>> +{
>> + NFSClient *client = arg;
>> + nfs_service(client->context, POLLOUT);
>> + nfs_set_events(client);
>> +}
>> +
>> +static void nfs_co_init_task(NFSClient *client, NFSRPC *task)
>> +{
>> + *task = (NFSRPC) {
>> + .co = qemu_coroutine_self(),
>> + };
>> +}
>> +
>> +static void nfs_co_generic_bh_cb(void *opaque)
>> +{
>> + NFSRPC *task = opaque;
>> + qemu_bh_delete(task->bh);
>> + qemu_coroutine_enter(task->co, NULL);
>> +}
>> +
>> +static void
>> +nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
>> + void *private_data)
>> +{
>> + NFSRPC *task = private_data;
>> + task->complete = 1;
>> + task->status = status;
>> + if (task->status > 0 && task->iov) {
>> + if (task->status <= task->iov->size) {
> It feel very odd to compare something named status with something named size.
>
>> + qemu_iovec_from_buf(task->iov, 0, data, task->status);
>> + } else {
>> + task->status = -EIO;
>> + }
>> + }
>> + if (task->status == 0 && task->st) {
>> + memcpy(task->st, data, sizeof(struct stat));
>> + }
>> + if (task->co) {
>> + task->bh = qemu_bh_new(nfs_co_generic_bh_cb, task);
>> + qemu_bh_schedule(task->bh);
>> + }
>> +}
>> +
>> +static int coroutine_fn nfs_co_readv(BlockDriverState *bs,
>> + int64_t sector_num, int nb_sectors,
>> + QEMUIOVector *iov)
>> +{
>> + NFSClient *client = bs->opaque;
>> + NFSRPC task;
>> +
>> + nfs_co_init_task(client, &task);
>> + task.iov = iov;
>> +
>> + if (nfs_pread_async(client->context, client->fh,
>> + sector_num * BDRV_SECTOR_SIZE,
>> + nb_sectors * BDRV_SECTOR_SIZE,
>> + nfs_co_generic_cb, &task) != 0) {
>> + return -ENOMEM;
>> + }
>> +
>> + while (!task.complete) {
>> + nfs_set_events(client);
>> + qemu_coroutine_yield();
>> + }
>> +
>> + if (task.status < 0) {
>> + return task.status;
>> + }
>> +
>> + /* zero pad short reads */
>> + if (task.status < iov->size) {
>> + qemu_iovec_memset(iov, task.status, 0, iov->size - task.status);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
>> + int64_t sector_num, int nb_sectors,
>> + QEMUIOVector *iov)
>> +{
>> + NFSClient *client = bs->opaque;
>> + NFSRPC task;
>> + char *buf = NULL;
>> +
>> + nfs_co_init_task(client, &task);
>> +
>> + buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
>> + qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
>> +
>> + if (nfs_pwrite_async(client->context, client->fh,
>> + sector_num * BDRV_SECTOR_SIZE,
>> + nb_sectors * BDRV_SECTOR_SIZE,
>> + buf, nfs_co_generic_cb, &task) != 0) {
>> + g_free(buf);
>> + return -ENOMEM;
>> + }
>> +
>> + while (!task.complete) {
>> + nfs_set_events(client);
>> + qemu_coroutine_yield();
>> + }
>> +
>> + g_free(buf);
>> +
>> + if (task.status != nb_sectors * BDRV_SECTOR_SIZE) {
>> + return task.status < 0 ? task.status : -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
>> +{
>> + NFSClient *client = bs->opaque;
>> + NFSRPC task;
>> +
>> + nfs_co_init_task(client, &task);
>> +
>> + if (nfs_fsync_async(client->context, client->fh, nfs_co_generic_cb,
>> + &task) != 0) {
>> + return -ENOMEM;
>> + }
>> +
>> + while (!task.complete) {
>> + nfs_set_events(client);
>> + qemu_coroutine_yield();
>> + }
>> +
>> + return task.status;
>> +}
>> +
>> +/* TODO Convert to fine grained options */
>> +static QemuOptsList runtime_opts = {
>> + .name = "nfs",
>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>> + .desc = {
>> + {
>> + .name = "filename",
>> + .type = QEMU_OPT_STRING,
>> + .help = "URL to the NFS file",
>> + },
>> + { /* end of list */ }
>> + },
>> +};
>> +
>> +static void nfs_client_close(NFSClient *client)
>> +{
>> + if (client->context) {
>> + if (client->fh) {
>> + nfs_close(client->context, client->fh);
>> + }
>> + qemu_aio_set_fd_handler(nfs_get_fd(client->context), NULL, NULL, NULL);
>> + nfs_destroy_context(client->context);
>> + }
>> + memset(client, 0, sizeof(NFSClient));
>> +}
>> +
>> +static void nfs_file_close(BlockDriverState *bs)
>> +{
>> + NFSClient *client = bs->opaque;
>> + nfs_client_close(client);
>> +}
>> +
>> +static int64_t nfs_.cilclient_open(NFSClient *client, const char *filename,
>> + int flags, Error **errp)
>> +{
>> + int ret = -EINVAL, i;
>> + 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");
>> + goto fail;
> Should this be goto out since we don't have done nfs_init_context yet ?
>> + }
>> + strp = strrchr(uri->path, '/');
>> + if (strp == NULL) {
>> + error_setg(errp, "Invalid URL specified");
>> + goto fail;
> goto out ?
>> + }
>> + file = g_strdup(strp);
>> + *strp = 0;
>> +
>> + client->context = nfs_init_context();
>> + if (client->context == NULL) {
>> + error_setg(errp, "Failed to init NFS context");
>> + goto fail;
>> + }
>> +
>> + qp = query_params_parse(uri->query);
>> + 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 fail;
>> + }
>> + if (!strncmp(qp->p[i].name, "uid", 3)) {
>> + nfs_set_uid(client->context, atoi(qp->p[i].value));
>> + } else if (!strncmp(qp->p[i].name, "gid", 3)) {
>> + nfs_set_gid(client->context, atoi(qp->p[i].value));
>> + } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
>> + nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
>> + } else {
>> + error_setg(errp, "Unknown NFS parameter name: %s",
>> + qp->p[i].name);
>> + goto fail;
>> + }
>> + }
>> +
>> + ret = nfs_mount(client->context, uri->server, uri->path);
>> + if (ret < 0) {
>> + error_setg(errp, "Failed to mount nfs share: %s",
>> + nfs_get_error(client->context));
>> + goto fail;
>> + }
>> +
>> + if (flags & O_CREAT) {
>> + ret = nfs_creat(client->context, file, 0600, &client->fh);
>> + if (ret < 0) {
>> + error_setg(errp, "Failed to create file: %s",
>> + nfs_get_error(client->context));
>> + goto fail;
>> + }
>> + } else {
>> + ret = nfs_open(client->context, file, flags, &client->fh);
>> + if (ret < 0) {
>> + error_setg(errp, "Failed to open file : %s",
>> + nfs_get_error(client->context));
>> + goto fail;
>> + }
>> + }
>> +
>> + ret = nfs_fstat(client->context, client->fh, &st);
>> + if (ret < 0) {
>> + error_setg(errp, "Failed to fstat file: %s",
>> + nfs_get_error(client->context));
>> + goto fail;
>> + }
>> +
>> + ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
>> + 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);
>> + g_free(file);
>> + return ret;
>> +}
>> +
>> +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;
>> +
>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>> + if (error_is_set(&local_err)) {
>> + qerror_report_err(local_err);
> I have seen more usage of error_propagate(errp, local_err); in QEMU code.
> Maybe I am missing the point.
>
>> + error_free(local_err);
>> + return -EINVAL;
>> + }
>> + ret = nfs_client_open(client, qemu_opt_get(opts, "filename"),
>> + (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
>> + errp);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> + bs->total_sectors = ret;
>> + return 0;
>> +}
>> +
>> +static int nfs_file_create(const char *url, QEMUOptionParameter *options,
>> + Error **errp)
>> +{
>> + int ret = 0;
>> + int64_t total_size = 0;
>> + NFSClient *client = g_malloc0(sizeof(NFSClient));
>> +
>> + /* Read out options */
>> + while (options && options->name) {
>> + if (!strcmp(options->name, "size")) {
>> + total_size = options->value.n;
>> + }
>> + options++;
>> + }
>> +
>> + ret = nfs_client_open(client, url, O_CREAT, errp);
>> + if (ret < 0) {
>> + goto out;
>> + }
>> + ret = nfs_ftruncate(client->context, client->fh, total_size);
>> +out:
> Should the out: label be after nfs_client_close(client); since the code will
> jump to it on nfs_client_open failure ?
You are right, but it doesn't hurt since you can call nfs_client_close multiple
times.
Peter
>
>> + nfs_client_close(client);
>> + g_free(client);
>> + return ret;
>> +}
>> +
>> +static int nfs_has_zero_init(BlockDriverState *bs)
>> +{
>> + NFSClient *client = bs->opaque;
>> + return client->has_zero_init;
>> +}
>> +
>> +static int64_t nfs_get_allocated_file_size(BlockDriverState *bs)
>> +{
>> + NFSClient *client = bs->opaque;
>> + NFSRPC task = {0};
>> + struct stat st;
>> +
>> + task.st = &st;
>> + if (nfs_fstat_async(client->context, client->fh, nfs_co_generic_cb,
>> + &task) != 0) {
>> + return -ENOMEM;
>> + }
>> +
>> + while (!task.complete) {
>> + nfs_set_events(client);
>> + qemu_aio_wait();
>> + }
>> +
>> + return (task.status < 0 ? task.status : st.st_blocks * st.st_blksize);
>> +}
>> +
>> +static int nfs_file_truncate(BlockDriverState *bs, int64_t offset)
>> +{
>> + NFSClient *client = bs->opaque;
>> + return nfs_ftruncate(client->context, client->fh, offset);
>> +}
>> +
>> +static BlockDriver bdrv_nfs = {
>> + .format_name = "nfs",
>> + .protocol_name = "nfs",
>> +
>> + .instance_size = sizeof(NFSClient),
>> + .bdrv_needs_filename = true,
>> + .bdrv_has_zero_init = nfs_has_zero_init,
>> + .bdrv_get_allocated_file_size = nfs_get_allocated_file_size,
>> + .bdrv_truncate = nfs_file_truncate,
>> +
>> + .bdrv_file_open = nfs_file_open,
>> + .bdrv_close = nfs_file_close,
>> + .bdrv_create = nfs_file_create,
>> +
>> + .bdrv_co_readv = nfs_co_readv,
>> + .bdrv_co_writev = nfs_co_writev,
>> + .bdrv_co_flush_to_disk = nfs_co_flush,
>> +};
>> +
>> +static void nfs_block_init(void)
>> +{
>> + bdrv_register(&bdrv_nfs);
>> +}
>> +
>> +block_init(nfs_block_init);
>> diff --git a/configure b/configure
>> index b472694..9ca0c63 100755
>> --- a/configure
>> +++ b/configure
>> @@ -251,6 +251,7 @@ vss_win32_sdk=""
>> win_sdk="no"
>> want_tools="yes"
>> libiscsi=""
>> +libnfs=""
>> coroutine=""
>> coroutine_pool=""
>> seccomp=""
>> @@ -840,6 +841,10 @@ for opt do
>> ;;
>> --enable-libiscsi) libiscsi="yes"
>> ;;
>> + --disable-libnfs) libnfs="no"
>> + ;;
>> + --enable-libnfs) libnfs="yes"
>> + ;;
>> --enable-profiler) profiler="yes"
>> ;;
>> --disable-cocoa) cocoa="no"
>> @@ -1229,6 +1234,8 @@ Advanced options (experts only):
>> --enable-rbd enable building the rados block device (rbd)
>> --disable-libiscsi disable iscsi support
>> --enable-libiscsi enable iscsi support
>> + --disable-libnfs disable nfs support
>> + --enable-libnfs enable nfs support
>> --disable-smartcard-nss disable smartcard nss support
>> --enable-smartcard-nss enable smartcard nss support
>> --disable-libusb disable libusb (for usb passthrough)
>> @@ -3600,6 +3607,20 @@ elif test "$debug" = "no" ; then
>> CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
>> fi
>>
>> +##########################################
>> +# Do we have libnfs
>> +if test "$libnfs" != "no" ; then
>> + if $pkg_config --atleast-version=1.8.91 libnfs; then
>> + libnfs="yes"
>> + libnfs_libs=$($pkg_config --libs libnfs)
>> + LIBS="$LIBS $libnfs_libs"
>> + else
>> + if test "$libnfs" = "yes" ; then
>> + feature_not_found "libnfs"
>> + fi
>> + libnfs="no"
>> + fi
>> +fi
>>
>> # Disable zero malloc errors for official releases unless explicitly told to
>> # enable/disable
>> @@ -3829,6 +3850,7 @@ echo "libiscsi support $libiscsi (1.4.0)"
>> else
>> echo "libiscsi support $libiscsi"
>> fi
>> +echo "libnfs support $libnfs"
>> echo "build guest agent $guest_agent"
>> echo "QGA VSS support $guest_agent_with_vss"
>> echo "seccomp support $seccomp"
>> @@ -4165,6 +4187,10 @@ if test "$libiscsi" = "yes" ; then
>> fi
>> fi
>>
>> +if test "$libnfs" = "yes" ; then
>> + echo "CONFIG_LIBNFS=y" >> $config_host_mak
>> +fi
>> +
>> if test "$seccomp" = "yes"; then
>> echo "CONFIG_SECCOMP=y" >> $config_host_mak
>> fi
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 05ced9d..7cfb5e5 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -4371,6 +4371,7 @@
>> # TODO gluster: Wait for structured options
>> # TODO iscsi: Wait for structured options
>> # TODO nbd: Should take InetSocketAddress for 'host'?
>> +# TODO nfs: Wait for structured options
>> # TODO rbd: Wait for structured options
>> # TODO sheepdog: Wait for structured options
>> # TODO ssh: Should take InetSocketAddress for 'host'?
>> --
>> 1.7.9.5
>>
>>
--
Mit freundlichen Grüßen
Peter Lieven
...........................................................
KAMP Netzwerkdienste GmbH
Vestische Str. 89-91 | 46117 Oberhausen
Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
pl@kamp.de | http://www.kamp.de
Geschäftsführer: Heiner Lante | Michael Lante
Amtsgericht Duisburg | HRB Nr. 12154
USt-Id-Nr.: DE 120607556
...........................................................
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-29 16:38 ` Peter Lieven
@ 2014-01-30 9:05 ` Stefan Hajnoczi
2014-01-30 9:12 ` Peter Lieven
0 siblings, 1 reply; 23+ messages in thread
From: Stefan Hajnoczi @ 2014-01-30 9:05 UTC (permalink / raw)
To: Peter Lieven
Cc: Benoît Canet, Kevin Wolf, Fam Zheng, ronniesahlberg,
Jeff Cody, qemu-devel, Max Reitz, owasserm, Federico Simoncelli,
Stefan Hajnoczi, Wenchao Xia
On Wed, Jan 29, 2014 at 05:38:29PM +0100, Peter Lieven wrote:
> On 29.01.2014 17:19, Benoît Canet wrote:
> >Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
> >>This patch adds native support for accessing images on NFS
> >>shares without the requirement to actually mount the entire
> >>NFS share on the host.
> >>
> >>NFS Images can simply be specified by an url of the form:
> >>nfs://<host>/<export>/<filename>[?param=value[¶m2=value2[&...]]]
> >>
> >>For example:
> >>qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2
> >>
> >>You need LibNFS from Ronnie Sahlberg available at:
> >> git://github.com/sahlberg/libnfs.git
> >>for this to work.
> >>
> >>During configure it is automatically probed for libnfs and support
> >>is enabled on-the-fly. You can forbid or enforce libnfs support
> >>with --disable-libnfs or --enable-libnfs respectively.
> >>
> >>Due to NFS restrictions you might need to execute your binaries
> >>as root, allow them to open priviledged ports (<1024) or specify
> >>insecure option on the NFS server.
> >>
> >>For additional information on ROOT vs. non-ROOT operation and URL
> >>format + parameters see:
> >> https://raw.github.com/sahlberg/libnfs/master/README
> >>
> >>Supported by qemu are the uid, gid and tcp-syncnt URL parameters.
> >>
> >>LibNFS currently support NFS version 3 only.
> >>
> >>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>---
> >> MAINTAINERS | 5 +
> >> block/Makefile.objs | 1 +
> >> block/nfs.c | 440 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >> configure | 26 +++
> >> qapi-schema.json | 1 +
> >> 5 files changed, 473 insertions(+)
> >> create mode 100644 block/nfs.c
> >>
> >>diff --git a/MAINTAINERS b/MAINTAINERS
> >>index fb53242..f8411f9 100644
> >>--- a/MAINTAINERS
> >>+++ b/MAINTAINERS
> >>@@ -936,6 +936,11 @@ M: Peter Lieven <pl@kamp.de>
> >> S: Supported
> >> F: block/iscsi.c
> >>+NFS
> >>+M: Peter Lieven <pl@kamp.de>
> >>+S: Maintained
> >>+F: block/nfs.c
> >>+
> >> SSH
> >> M: Richard W.M. Jones <rjones@redhat.com>
> >> S: Supported
> >>diff --git a/block/Makefile.objs b/block/Makefile.objs
> >>index 4e8c91e..e254a21 100644
> >>--- a/block/Makefile.objs
> >>+++ b/block/Makefile.objs
> >>@@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> >> ifeq ($(CONFIG_POSIX),y)
> >> block-obj-y += nbd.o nbd-client.o sheepdog.o
> >> block-obj-$(CONFIG_LIBISCSI) += iscsi.o
> >>+block-obj-$(CONFIG_LIBNFS) += nfs.o
> >> block-obj-$(CONFIG_CURL) += curl.o
> >> block-obj-$(CONFIG_RBD) += rbd.o
> >> block-obj-$(CONFIG_GLUSTERFS) += gluster.o
> >>diff --git a/block/nfs.c b/block/nfs.c
> >>new file mode 100644
> >>index 0000000..2bbf7a2
> >>--- /dev/null
> >>+++ b/block/nfs.c
> >>@@ -0,0 +1,440 @@
> >>+/*
> >>+ * QEMU Block driver for native access to files on NFS shares
> >>+ *
> >>+ * Copyright (c) 2014 Peter Lieven <pl@kamp.de>
> >>+ *
> >>+ * Permission is hereby granted, free of charge, to any person obtaining a copy
> >>+ * of this software and associated documentation files (the "Software"), to deal
> >>+ * in the Software without restriction, including without limitation the rights
> >>+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> >>+ * copies of the Software, and to permit persons to whom the Software is
> >>+ * furnished to do so, subject to the following conditions:
> >>+ *
> >>+ * The above copyright notice and this permission notice shall be included in
> >>+ * all copies or substantial portions of the Software.
> >>+ *
> >>+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >>+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >>+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >>+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >>+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> >>+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> >>+ * THE SOFTWARE.
> >>+ */
> >>+
> >>+#include "config-host.h"
> >>+
> >>+#include <poll.h>
> >>+#include "qemu-common.h"
> >>+#include "qemu/config-file.h"
> >>+#include "qemu/error-report.h"
> >>+#include "block/block_int.h"
> >>+#include "trace.h"
> >>+#include "qemu/iov.h"
> >>+#include "qemu/uri.h"
> >>+#include "sysemu/sysemu.h"
> >>+#include <nfsc/libnfs.h>
> >>+
> >>+typedef struct NFSClient {
> >>+ struct nfs_context *context;
> >>+ struct nfsfh *fh;
> >>+ int events;
> >>+ bool has_zero_init;
> >>+} NFSClient;
> >>+
> >>+typedef struct NFSRPC {
> >>+ int status;
> >>+ int complete;
> >>+ QEMUIOVector *iov;
> >>+ struct stat *st;
> >>+ Coroutine *co;
> >>+ QEMUBH *bh;
> >>+} NFSRPC;
> >>+
> >>+static void nfs_process_read(void *arg);
> >>+static void nfs_process_write(void *arg);
> >>+
> >>+static void nfs_set_events(NFSClient *client)
> >>+{
> >>+ int ev = nfs_which_events(client->context);
> >>+ if (ev != client->events) {
> >>+ qemu_aio_set_fd_handler(nfs_get_fd(client->context),
> >>+ (ev & POLLIN) ? nfs_process_read : NULL,
> >>+ (ev & POLLOUT) ? nfs_process_write : NULL,
> >>+ client);
> >>+
> >>+ }
> >>+ client->events = ev;
> >>+}
> >>+
> >>+static void nfs_process_read(void *arg)
> >>+{
> >>+ NFSClient *client = arg;
> >>+ nfs_service(client->context, POLLIN);
> >>+ nfs_set_events(client);
> >>+}
> >>+
> >>+static void nfs_process_write(void *arg)
> >>+{
> >>+ NFSClient *client = arg;
> >>+ nfs_service(client->context, POLLOUT);
> >>+ nfs_set_events(client);
> >>+}
> >>+
> >>+static void nfs_co_init_task(NFSClient *client, NFSRPC *task)
> >>+{
> >>+ *task = (NFSRPC) {
> >>+ .co = qemu_coroutine_self(),
> >>+ };
> >>+}
> >>+
> >>+static void nfs_co_generic_bh_cb(void *opaque)
> >>+{
> >>+ NFSRPC *task = opaque;
> >>+ qemu_bh_delete(task->bh);
> >>+ qemu_coroutine_enter(task->co, NULL);
> >>+}
> >>+
> >>+static void
> >>+nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
> >>+ void *private_data)
> >>+{
> >>+ NFSRPC *task = private_data;
> >>+ task->complete = 1;
> >>+ task->status = status;
> >>+ if (task->status > 0 && task->iov) {
> >>+ if (task->status <= task->iov->size) {
> >It feel very odd to compare something named status with something named size.
> >
> >>+ qemu_iovec_from_buf(task->iov, 0, data, task->status);
> >>+ } else {
> >>+ task->status = -EIO;
> >>+ }
> >>+ }
> >>+ if (task->status == 0 && task->st) {
> >>+ memcpy(task->st, data, sizeof(struct stat));
> >>+ }
> >>+ if (task->co) {
> >>+ task->bh = qemu_bh_new(nfs_co_generic_bh_cb, task);
> >>+ qemu_bh_schedule(task->bh);
> >>+ }
> >>+}
> >>+
> >>+static int coroutine_fn nfs_co_readv(BlockDriverState *bs,
> >>+ int64_t sector_num, int nb_sectors,
> >>+ QEMUIOVector *iov)
> >>+{
> >>+ NFSClient *client = bs->opaque;
> >>+ NFSRPC task;
> >>+
> >>+ nfs_co_init_task(client, &task);
> >>+ task.iov = iov;
> >>+
> >>+ if (nfs_pread_async(client->context, client->fh,
> >>+ sector_num * BDRV_SECTOR_SIZE,
> >>+ nb_sectors * BDRV_SECTOR_SIZE,
> >>+ nfs_co_generic_cb, &task) != 0) {
> >>+ return -ENOMEM;
> >>+ }
> >>+
> >>+ while (!task.complete) {
> >>+ nfs_set_events(client);
> >>+ qemu_coroutine_yield();
> >>+ }
> >>+
> >>+ if (task.status < 0) {
> >>+ return task.status;
> >>+ }
> >>+
> >>+ /* zero pad short reads */
> >>+ if (task.status < iov->size) {
> >>+ qemu_iovec_memset(iov, task.status, 0, iov->size - task.status);
> >>+ }
> >>+
> >>+ return 0;
> >>+}
> >>+
> >>+static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
> >>+ int64_t sector_num, int nb_sectors,
> >>+ QEMUIOVector *iov)
> >>+{
> >>+ NFSClient *client = bs->opaque;
> >>+ NFSRPC task;
> >>+ char *buf = NULL;
> >>+
> >>+ nfs_co_init_task(client, &task);
> >>+
> >>+ buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
> >>+ qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
> >>+
> >>+ if (nfs_pwrite_async(client->context, client->fh,
> >>+ sector_num * BDRV_SECTOR_SIZE,
> >>+ nb_sectors * BDRV_SECTOR_SIZE,
> >>+ buf, nfs_co_generic_cb, &task) != 0) {
> >>+ g_free(buf);
> >>+ return -ENOMEM;
> >>+ }
> >>+
> >>+ while (!task.complete) {
> >>+ nfs_set_events(client);
> >>+ qemu_coroutine_yield();
> >>+ }
> >>+
> >>+ g_free(buf);
> >>+
> >>+ if (task.status != nb_sectors * BDRV_SECTOR_SIZE) {
> >>+ return task.status < 0 ? task.status : -EIO;
> >>+ }
> >>+
> >>+ return 0;
> >>+}
> >>+
> >>+static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
> >>+{
> >>+ NFSClient *client = bs->opaque;
> >>+ NFSRPC task;
> >>+
> >>+ nfs_co_init_task(client, &task);
> >>+
> >>+ if (nfs_fsync_async(client->context, client->fh, nfs_co_generic_cb,
> >>+ &task) != 0) {
> >>+ return -ENOMEM;
> >>+ }
> >>+
> >>+ while (!task.complete) {
> >>+ nfs_set_events(client);
> >>+ qemu_coroutine_yield();
> >>+ }
> >>+
> >>+ return task.status;
> >>+}
> >>+
> >>+/* TODO Convert to fine grained options */
> >>+static QemuOptsList runtime_opts = {
> >>+ .name = "nfs",
> >>+ .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> >>+ .desc = {
> >>+ {
> >>+ .name = "filename",
> >>+ .type = QEMU_OPT_STRING,
> >>+ .help = "URL to the NFS file",
> >>+ },
> >>+ { /* end of list */ }
> >>+ },
> >>+};
> >>+
> >>+static void nfs_client_close(NFSClient *client)
> >>+{
> >>+ if (client->context) {
> >>+ if (client->fh) {
> >>+ nfs_close(client->context, client->fh);
> >>+ }
> >>+ qemu_aio_set_fd_handler(nfs_get_fd(client->context), NULL, NULL, NULL);
> >>+ nfs_destroy_context(client->context);
> >>+ }
> >>+ memset(client, 0, sizeof(NFSClient));
> >>+}
> >>+
> >>+static void nfs_file_close(BlockDriverState *bs)
> >>+{
> >>+ NFSClient *client = bs->opaque;
> >>+ nfs_client_close(client);
> >>+}
> >>+
> >>+static int64_t nfs_.cilclient_open(NFSClient *client, const char *filename,
> >>+ int flags, Error **errp)
> >>+{
> >>+ int ret = -EINVAL, i;
> >>+ 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");
> >>+ goto fail;
> >Should this be goto out since we don't have done nfs_init_context yet ?
> >>+ }
> >>+ strp = strrchr(uri->path, '/');
> >>+ if (strp == NULL) {
> >>+ error_setg(errp, "Invalid URL specified");
> >>+ goto fail;
> >goto out ?
> >>+ }
> >>+ file = g_strdup(strp);
> >>+ *strp = 0;
> >>+
> >>+ client->context = nfs_init_context();
> >>+ if (client->context == NULL) {
> >>+ error_setg(errp, "Failed to init NFS context");
> >>+ goto fail;
> >>+ }
> >>+
> >>+ qp = query_params_parse(uri->query);
> >>+ 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 fail;
> >>+ }
> >>+ if (!strncmp(qp->p[i].name, "uid", 3)) {
> >>+ nfs_set_uid(client->context, atoi(qp->p[i].value));
> >>+ } else if (!strncmp(qp->p[i].name, "gid", 3)) {
> >>+ nfs_set_gid(client->context, atoi(qp->p[i].value));
> >>+ } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
> >>+ nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
> >>+ } else {
> >>+ error_setg(errp, "Unknown NFS parameter name: %s",
> >>+ qp->p[i].name);
> >>+ goto fail;
> >>+ }
> >>+ }
> >>+
> >>+ ret = nfs_mount(client->context, uri->server, uri->path);
> >>+ if (ret < 0) {
> >>+ error_setg(errp, "Failed to mount nfs share: %s",
> >>+ nfs_get_error(client->context));
> >>+ goto fail;
> >>+ }
> >>+
> >>+ if (flags & O_CREAT) {
> >>+ ret = nfs_creat(client->context, file, 0600, &client->fh);
> >>+ if (ret < 0) {
> >>+ error_setg(errp, "Failed to create file: %s",
> >>+ nfs_get_error(client->context));
> >>+ goto fail;
> >>+ }
> >>+ } else {
> >>+ ret = nfs_open(client->context, file, flags, &client->fh);
> >>+ if (ret < 0) {
> >>+ error_setg(errp, "Failed to open file : %s",
> >>+ nfs_get_error(client->context));
> >>+ goto fail;
> >>+ }
> >>+ }
> >>+
> >>+ ret = nfs_fstat(client->context, client->fh, &st);
> >>+ if (ret < 0) {
> >>+ error_setg(errp, "Failed to fstat file: %s",
> >>+ nfs_get_error(client->context));
> >>+ goto fail;
> >>+ }
> >>+
> >>+ ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
> >>+ 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);
> >>+ g_free(file);
> >>+ return ret;
> >>+}
> >>+
> >>+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;
> >>+
> >>+ opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> >>+ qemu_opts_absorb_qdict(opts, options, &local_err);
> >>+ if (error_is_set(&local_err)) {
> >>+ qerror_report_err(local_err);
> >I have seen more usage of error_propagate(errp, local_err); in QEMU code.
> >Maybe I am missing the point.
> >
> >>+ error_free(local_err);
> >>+ return -EINVAL;
> >>+ }
> >>+ ret = nfs_client_open(client, qemu_opt_get(opts, "filename"),
> >>+ (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
> >>+ errp);
> >>+ if (ret < 0) {
> >>+ return ret;
> >>+ }
> >>+ bs->total_sectors = ret;
> >>+ return 0;
> >>+}
> >>+
> >>+static int nfs_file_create(const char *url, QEMUOptionParameter *options,
> >>+ Error **errp)
> >>+{
> >>+ int ret = 0;
> >>+ int64_t total_size = 0;
> >>+ NFSClient *client = g_malloc0(sizeof(NFSClient));
> >>+
> >>+ /* Read out options */
> >>+ while (options && options->name) {
> >>+ if (!strcmp(options->name, "size")) {
> >>+ total_size = options->value.n;
> >>+ }
> >>+ options++;
> >>+ }
> >>+
> >>+ ret = nfs_client_open(client, url, O_CREAT, errp);
> >>+ if (ret < 0) {
> >>+ goto out;
> >>+ }
> >>+ ret = nfs_ftruncate(client->context, client->fh, total_size);
> >>+out:
> >Should the out: label be after nfs_client_close(client); since the code will
> >jump to it on nfs_client_open failure ?
> You are right, but it doesn't hurt since you can call nfs_client_close multiple
> times.
Will you send another revision addressing the other comments?
Stefan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-30 9:05 ` Stefan Hajnoczi
@ 2014-01-30 9:12 ` Peter Lieven
2014-01-30 14:23 ` Stefan Hajnoczi
0 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2014-01-30 9:12 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Benoît Canet, Kevin Wolf, Fam Zheng, ronniesahlberg,
Jeff Cody, qemu-devel, Max Reitz, owasserm, Federico Simoncelli,
Stefan Hajnoczi, Wenchao Xia
Am 30.01.2014 um 10:05 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
> On Wed, Jan 29, 2014 at 05:38:29PM +0100, Peter Lieven wrote:
>> On 29.01.2014 17:19, Benoît Canet wrote:
>>> Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
>>>> This patch adds native support for accessing images on NFS
>>>> shares without the requirement to actually mount the entire
>>>> NFS share on the host.
>>>>
>>>> NFS Images can simply be specified by an url of the form:
>>>> nfs://<host>/<export>/<filename>[?param=value[¶m2=value2[&...]]]
>>>>
>>>> For example:
>>>> qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2
>>>>
>>>> You need LibNFS from Ronnie Sahlberg available at:
>>>> git://github.com/sahlberg/libnfs.git
>>>> for this to work.
>>>>
>>>> During configure it is automatically probed for libnfs and support
>>>> is enabled on-the-fly. You can forbid or enforce libnfs support
>>>> with --disable-libnfs or --enable-libnfs respectively.
>>>>
>>>> Due to NFS restrictions you might need to execute your binaries
>>>> as root, allow them to open priviledged ports (<1024) or specify
>>>> insecure option on the NFS server.
>>>>
>>>> For additional information on ROOT vs. non-ROOT operation and URL
>>>> format + parameters see:
>>>> https://raw.github.com/sahlberg/libnfs/master/README
>>>>
>>>> Supported by qemu are the uid, gid and tcp-syncnt URL parameters.
>>>>
>>>> LibNFS currently support NFS version 3 only.
>>>>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>> MAINTAINERS | 5 +
>>>> block/Makefile.objs | 1 +
>>>> block/nfs.c | 440 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> configure | 26 +++
>>>> qapi-schema.json | 1 +
>>>> 5 files changed, 473 insertions(+)
>>>> create mode 100644 block/nfs.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index fb53242..f8411f9 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -936,6 +936,11 @@ M: Peter Lieven <pl@kamp.de>
>>>> S: Supported
>>>> F: block/iscsi.c
>>>> +NFS
>>>> +M: Peter Lieven <pl@kamp.de>
>>>> +S: Maintained
>>>> +F: block/nfs.c
>>>> +
>>>> SSH
>>>> M: Richard W.M. Jones <rjones@redhat.com>
>>>> S: Supported
>>>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>>>> index 4e8c91e..e254a21 100644
>>>> --- a/block/Makefile.objs
>>>> +++ b/block/Makefile.objs
>>>> @@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>>>> ifeq ($(CONFIG_POSIX),y)
>>>> block-obj-y += nbd.o nbd-client.o sheepdog.o
>>>> block-obj-$(CONFIG_LIBISCSI) += iscsi.o
>>>> +block-obj-$(CONFIG_LIBNFS) += nfs.o
>>>> block-obj-$(CONFIG_CURL) += curl.o
>>>> block-obj-$(CONFIG_RBD) += rbd.o
>>>> block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>>>> diff --git a/block/nfs.c b/block/nfs.c
>>>> new file mode 100644
>>>> index 0000000..2bbf7a2
>>>> --- /dev/null
>>>> +++ b/block/nfs.c
>>>> @@ -0,0 +1,440 @@
>>>> +/*
>>>> + * QEMU Block driver for native access to files on NFS shares
>>>> + *
>>>> + * Copyright (c) 2014 Peter Lieven <pl@kamp.de>
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>> + * of this software and associated documentation files (the "Software"), to deal
>>>> + * in the Software without restriction, including without limitation the rights
>>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>>> + * copies of the Software, and to permit persons to whom the Software is
>>>> + * furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>> + * THE SOFTWARE.
>>>> + */
>>>> +
>>>> +#include "config-host.h"
>>>> +
>>>> +#include <poll.h>
>>>> +#include "qemu-common.h"
>>>> +#include "qemu/config-file.h"
>>>> +#include "qemu/error-report.h"
>>>> +#include "block/block_int.h"
>>>> +#include "trace.h"
>>>> +#include "qemu/iov.h"
>>>> +#include "qemu/uri.h"
>>>> +#include "sysemu/sysemu.h"
>>>> +#include <nfsc/libnfs.h>
>>>> +
>>>> +typedef struct NFSClient {
>>>> + struct nfs_context *context;
>>>> + struct nfsfh *fh;
>>>> + int events;
>>>> + bool has_zero_init;
>>>> +} NFSClient;
>>>> +
>>>> +typedef struct NFSRPC {
>>>> + int status;
>>>> + int complete;
>>>> + QEMUIOVector *iov;
>>>> + struct stat *st;
>>>> + Coroutine *co;
>>>> + QEMUBH *bh;
>>>> +} NFSRPC;
>>>> +
>>>> +static void nfs_process_read(void *arg);
>>>> +static void nfs_process_write(void *arg);
>>>> +
>>>> +static void nfs_set_events(NFSClient *client)
>>>> +{
>>>> + int ev = nfs_which_events(client->context);
>>>> + if (ev != client->events) {
>>>> + qemu_aio_set_fd_handler(nfs_get_fd(client->context),
>>>> + (ev & POLLIN) ? nfs_process_read : NULL,
>>>> + (ev & POLLOUT) ? nfs_process_write : NULL,
>>>> + client);
>>>> +
>>>> + }
>>>> + client->events = ev;
>>>> +}
>>>> +
>>>> +static void nfs_process_read(void *arg)
>>>> +{
>>>> + NFSClient *client = arg;
>>>> + nfs_service(client->context, POLLIN);
>>>> + nfs_set_events(client);
>>>> +}
>>>> +
>>>> +static void nfs_process_write(void *arg)
>>>> +{
>>>> + NFSClient *client = arg;
>>>> + nfs_service(client->context, POLLOUT);
>>>> + nfs_set_events(client);
>>>> +}
>>>> +
>>>> +static void nfs_co_init_task(NFSClient *client, NFSRPC *task)
>>>> +{
>>>> + *task = (NFSRPC) {
>>>> + .co = qemu_coroutine_self(),
>>>> + };
>>>> +}
>>>> +
>>>> +static void nfs_co_generic_bh_cb(void *opaque)
>>>> +{
>>>> + NFSRPC *task = opaque;
>>>> + qemu_bh_delete(task->bh);
>>>> + qemu_coroutine_enter(task->co, NULL);
>>>> +}
>>>> +
>>>> +static void
>>>> +nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
>>>> + void *private_data)
>>>> +{
>>>> + NFSRPC *task = private_data;
>>>> + task->complete = 1;
>>>> + task->status = status;
>>>> + if (task->status > 0 && task->iov) {
>>>> + if (task->status <= task->iov->size) {
>>> It feel very odd to compare something named status with something named size.
>>>
>>>> + qemu_iovec_from_buf(task->iov, 0, data, task->status);
>>>> + } else {
>>>> + task->status = -EIO;
>>>> + }
>>>> + }
>>>> + if (task->status == 0 && task->st) {
>>>> + memcpy(task->st, data, sizeof(struct stat));
>>>> + }
>>>> + if (task->co) {
>>>> + task->bh = qemu_bh_new(nfs_co_generic_bh_cb, task);
>>>> + qemu_bh_schedule(task->bh);
>>>> + }
>>>> +}
>>>> +
>>>> +static int coroutine_fn nfs_co_readv(BlockDriverState *bs,
>>>> + int64_t sector_num, int nb_sectors,
>>>> + QEMUIOVector *iov)
>>>> +{
>>>> + NFSClient *client = bs->opaque;
>>>> + NFSRPC task;
>>>> +
>>>> + nfs_co_init_task(client, &task);
>>>> + task.iov = iov;
>>>> +
>>>> + if (nfs_pread_async(client->context, client->fh,
>>>> + sector_num * BDRV_SECTOR_SIZE,
>>>> + nb_sectors * BDRV_SECTOR_SIZE,
>>>> + nfs_co_generic_cb, &task) != 0) {
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + while (!task.complete) {
>>>> + nfs_set_events(client);
>>>> + qemu_coroutine_yield();
>>>> + }
>>>> +
>>>> + if (task.status < 0) {
>>>> + return task.status;
>>>> + }
>>>> +
>>>> + /* zero pad short reads */
>>>> + if (task.status < iov->size) {
>>>> + qemu_iovec_memset(iov, task.status, 0, iov->size - task.status);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
>>>> + int64_t sector_num, int nb_sectors,
>>>> + QEMUIOVector *iov)
>>>> +{
>>>> + NFSClient *client = bs->opaque;
>>>> + NFSRPC task;
>>>> + char *buf = NULL;
>>>> +
>>>> + nfs_co_init_task(client, &task);
>>>> +
>>>> + buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
>>>> + qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
>>>> +
>>>> + if (nfs_pwrite_async(client->context, client->fh,
>>>> + sector_num * BDRV_SECTOR_SIZE,
>>>> + nb_sectors * BDRV_SECTOR_SIZE,
>>>> + buf, nfs_co_generic_cb, &task) != 0) {
>>>> + g_free(buf);
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + while (!task.complete) {
>>>> + nfs_set_events(client);
>>>> + qemu_coroutine_yield();
>>>> + }
>>>> +
>>>> + g_free(buf);
>>>> +
>>>> + if (task.status != nb_sectors * BDRV_SECTOR_SIZE) {
>>>> + return task.status < 0 ? task.status : -EIO;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
>>>> +{
>>>> + NFSClient *client = bs->opaque;
>>>> + NFSRPC task;
>>>> +
>>>> + nfs_co_init_task(client, &task);
>>>> +
>>>> + if (nfs_fsync_async(client->context, client->fh, nfs_co_generic_cb,
>>>> + &task) != 0) {
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + while (!task.complete) {
>>>> + nfs_set_events(client);
>>>> + qemu_coroutine_yield();
>>>> + }
>>>> +
>>>> + return task.status;
>>>> +}
>>>> +
>>>> +/* TODO Convert to fine grained options */
>>>> +static QemuOptsList runtime_opts = {
>>>> + .name = "nfs",
>>>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>>>> + .desc = {
>>>> + {
>>>> + .name = "filename",
>>>> + .type = QEMU_OPT_STRING,
>>>> + .help = "URL to the NFS file",
>>>> + },
>>>> + { /* end of list */ }
>>>> + },
>>>> +};
>>>> +
>>>> +static void nfs_client_close(NFSClient *client)
>>>> +{
>>>> + if (client->context) {
>>>> + if (client->fh) {
>>>> + nfs_close(client->context, client->fh);
>>>> + }
>>>> + qemu_aio_set_fd_handler(nfs_get_fd(client->context), NULL, NULL, NULL);
>>>> + nfs_destroy_context(client->context);
>>>> + }
>>>> + memset(client, 0, sizeof(NFSClient));
>>>> +}
>>>> +
>>>> +static void nfs_file_close(BlockDriverState *bs)
>>>> +{
>>>> + NFSClient *client = bs->opaque;
>>>> + nfs_client_close(client);
>>>> +}
>>>> +
>>>> +static int64_t nfs_.cilclient_open(NFSClient *client, const char *filename,
>>>> + int flags, Error **errp)
>>>> +{
>>>> + int ret = -EINVAL, i;
>>>> + 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");
>>>> + goto fail;
>>> Should this be goto out since we don't have done nfs_init_context yet ?
>>>> + }
>>>> + strp = strrchr(uri->path, '/');
>>>> + if (strp == NULL) {
>>>> + error_setg(errp, "Invalid URL specified");
>>>> + goto fail;
>>> goto out ?
>>>> + }
>>>> + file = g_strdup(strp);
>>>> + *strp = 0;
>>>> +
>>>> + client->context = nfs_init_context();
>>>> + if (client->context == NULL) {
>>>> + error_setg(errp, "Failed to init NFS context");
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + qp = query_params_parse(uri->query);
>>>> + 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 fail;
>>>> + }
>>>> + if (!strncmp(qp->p[i].name, "uid", 3)) {
>>>> + nfs_set_uid(client->context, atoi(qp->p[i].value));
>>>> + } else if (!strncmp(qp->p[i].name, "gid", 3)) {
>>>> + nfs_set_gid(client->context, atoi(qp->p[i].value));
>>>> + } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
>>>> + nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
>>>> + } else {
>>>> + error_setg(errp, "Unknown NFS parameter name: %s",
>>>> + qp->p[i].name);
>>>> + goto fail;
>>>> + }
>>>> + }
>>>> +
>>>> + ret = nfs_mount(client->context, uri->server, uri->path);
>>>> + if (ret < 0) {
>>>> + error_setg(errp, "Failed to mount nfs share: %s",
>>>> + nfs_get_error(client->context));
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + if (flags & O_CREAT) {
>>>> + ret = nfs_creat(client->context, file, 0600, &client->fh);
>>>> + if (ret < 0) {
>>>> + error_setg(errp, "Failed to create file: %s",
>>>> + nfs_get_error(client->context));
>>>> + goto fail;
>>>> + }
>>>> + } else {
>>>> + ret = nfs_open(client->context, file, flags, &client->fh);
>>>> + if (ret < 0) {
>>>> + error_setg(errp, "Failed to open file : %s",
>>>> + nfs_get_error(client->context));
>>>> + goto fail;
>>>> + }
>>>> + }
>>>> +
>>>> + ret = nfs_fstat(client->context, client->fh, &st);
>>>> + if (ret < 0) {
>>>> + error_setg(errp, "Failed to fstat file: %s",
>>>> + nfs_get_error(client->context));
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
>>>> + 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);
>>>> + g_free(file);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +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;
>>>> +
>>>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>>>> + if (error_is_set(&local_err)) {
>>>> + qerror_report_err(local_err);
>>> I have seen more usage of error_propagate(errp, local_err); in QEMU code.
>>> Maybe I am missing the point.
>>>
>>>> + error_free(local_err);
>>>> + return -EINVAL;
>>>> + }
>>>> + ret = nfs_client_open(client, qemu_opt_get(opts, "filename"),
>>>> + (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
>>>> + errp);
>>>> + if (ret < 0) {
>>>> + return ret;
>>>> + }
>>>> + bs->total_sectors = ret;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int nfs_file_create(const char *url, QEMUOptionParameter *options,
>>>> + Error **errp)
>>>> +{
>>>> + int ret = 0;
>>>> + int64_t total_size = 0;
>>>> + NFSClient *client = g_malloc0(sizeof(NFSClient));
>>>> +
>>>> + /* Read out options */
>>>> + while (options && options->name) {
>>>> + if (!strcmp(options->name, "size")) {
>>>> + total_size = options->value.n;
>>>> + }
>>>> + options++;
>>>> + }
>>>> +
>>>> + ret = nfs_client_open(client, url, O_CREAT, errp);
>>>> + if (ret < 0) {
>>>> + goto out;
>>>> + }
>>>> + ret = nfs_ftruncate(client->context, client->fh, total_size);
>>>> +out:
>>> Should the out: label be after nfs_client_close(client); since the code will
>>> jump to it on nfs_client_open failure ?
>> You are right, but it doesn't hurt since you can call nfs_client_close multiple
>> times.
>
> Will you send another revision addressing the other comments?
Sorry, which other comments are you referring to?
Have you been able to test the patch without the bad commit?
BR,
Peter
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-29 16:19 ` Benoît Canet
2014-01-29 16:38 ` Peter Lieven
@ 2014-01-30 14:22 ` Stefan Hajnoczi
2014-01-30 21:35 ` Peter Lieven
2014-01-31 11:36 ` Peter Lieven
2 siblings, 1 reply; 23+ messages in thread
From: Stefan Hajnoczi @ 2014-01-30 14:22 UTC (permalink / raw)
To: Benoît Canet
Cc: Kevin Wolf, Fam Zheng, ronniesahlberg, Jeff Cody, Peter Lieven,
qemu-devel, Max Reitz, owasserm, Federico Simoncelli,
Stefan Hajnoczi, Wenchao Xia
On Wed, Jan 29, 2014 at 05:19:59PM +0100, Benoît Canet wrote:
> Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
Hi Peter,
If I read your reply to Benoit correctly, you only addressed the
questions about nfs_client_close(). Here are Benoits other comments and
my thoughts on them:
> > +static void
> > +nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
> > + void *private_data)
> > +{
> > + NFSRPC *task = private_data;
> > + task->complete = 1;
> > + task->status = status;
> > + if (task->status > 0 && task->iov) {
> > + if (task->status <= task->iov->size) {
> It feel very odd to compare something named status with something named size.
Maybe the argument name isn't ideal. Something like 'ret' or 'result'
would be more commonplace.
Not critical but would be nice to change.
> > +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;
> > +
> > + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> > + qemu_opts_absorb_qdict(opts, options, &local_err);
> > + if (error_is_set(&local_err)) {
> > + qerror_report_err(local_err);
> I have seen more usage of error_propagate(errp, local_err); in QEMU code.
> Maybe I am missing the point.
Yes, I think you are right. The Error should be propagated to the
caller. It's not clear to me whether we can ever get an error from
qemu_opts_absorb_qdict() in this call site though.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-30 9:12 ` Peter Lieven
@ 2014-01-30 14:23 ` Stefan Hajnoczi
2014-01-30 21:33 ` Peter Lieven
0 siblings, 1 reply; 23+ messages in thread
From: Stefan Hajnoczi @ 2014-01-30 14:23 UTC (permalink / raw)
To: Peter Lieven
Cc: Benoît Canet, Kevin Wolf, Fam Zheng, ronniesahlberg,
Jeff Cody, qemu-devel, Max Reitz, owasserm, Federico Simoncelli,
Stefan Hajnoczi, Wenchao Xia
On Thu, Jan 30, 2014 at 10:12:35AM +0100, Peter Lieven wrote:
>
> Am 30.01.2014 um 10:05 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
>
> > On Wed, Jan 29, 2014 at 05:38:29PM +0100, Peter Lieven wrote:
> >> On 29.01.2014 17:19, Benoît Canet wrote:
> >>> Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
> >>>> This patch adds native support for accessing images on NFS
> >>>> shares without the requirement to actually mount the entire
> >>>> NFS share on the host.
> >>>>
> >>>> NFS Images can simply be specified by an url of the form:
> >>>> nfs://<host>/<export>/<filename>[?param=value[¶m2=value2[&...]]]
> >>>>
> >>>> For example:
> >>>> qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2
> >>>>
> >>>> You need LibNFS from Ronnie Sahlberg available at:
> >>>> git://github.com/sahlberg/libnfs.git
> >>>> for this to work.
> >>>>
> >>>> During configure it is automatically probed for libnfs and support
> >>>> is enabled on-the-fly. You can forbid or enforce libnfs support
> >>>> with --disable-libnfs or --enable-libnfs respectively.
> >>>>
> >>>> Due to NFS restrictions you might need to execute your binaries
> >>>> as root, allow them to open priviledged ports (<1024) or specify
> >>>> insecure option on the NFS server.
> >>>>
> >>>> For additional information on ROOT vs. non-ROOT operation and URL
> >>>> format + parameters see:
> >>>> https://raw.github.com/sahlberg/libnfs/master/README
> >>>>
> >>>> Supported by qemu are the uid, gid and tcp-syncnt URL parameters.
> >>>>
> >>>> LibNFS currently support NFS version 3 only.
> >>>>
> >>>> Signed-off-by: Peter Lieven <pl@kamp.de>
> >>>> ---
> >>>> MAINTAINERS | 5 +
> >>>> block/Makefile.objs | 1 +
> >>>> block/nfs.c | 440 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>> configure | 26 +++
> >>>> qapi-schema.json | 1 +
> >>>> 5 files changed, 473 insertions(+)
> >>>> create mode 100644 block/nfs.c
> >>>>
> >>>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>>> index fb53242..f8411f9 100644
> >>>> --- a/MAINTAINERS
> >>>> +++ b/MAINTAINERS
> >>>> @@ -936,6 +936,11 @@ M: Peter Lieven <pl@kamp.de>
> >>>> S: Supported
> >>>> F: block/iscsi.c
> >>>> +NFS
> >>>> +M: Peter Lieven <pl@kamp.de>
> >>>> +S: Maintained
> >>>> +F: block/nfs.c
> >>>> +
> >>>> SSH
> >>>> M: Richard W.M. Jones <rjones@redhat.com>
> >>>> S: Supported
> >>>> diff --git a/block/Makefile.objs b/block/Makefile.objs
> >>>> index 4e8c91e..e254a21 100644
> >>>> --- a/block/Makefile.objs
> >>>> +++ b/block/Makefile.objs
> >>>> @@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> >>>> ifeq ($(CONFIG_POSIX),y)
> >>>> block-obj-y += nbd.o nbd-client.o sheepdog.o
> >>>> block-obj-$(CONFIG_LIBISCSI) += iscsi.o
> >>>> +block-obj-$(CONFIG_LIBNFS) += nfs.o
> >>>> block-obj-$(CONFIG_CURL) += curl.o
> >>>> block-obj-$(CONFIG_RBD) += rbd.o
> >>>> block-obj-$(CONFIG_GLUSTERFS) += gluster.o
> >>>> diff --git a/block/nfs.c b/block/nfs.c
> >>>> new file mode 100644
> >>>> index 0000000..2bbf7a2
> >>>> --- /dev/null
> >>>> +++ b/block/nfs.c
> >>>> @@ -0,0 +1,440 @@
> >>>> +/*
> >>>> + * QEMU Block driver for native access to files on NFS shares
> >>>> + *
> >>>> + * Copyright (c) 2014 Peter Lieven <pl@kamp.de>
> >>>> + *
> >>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> >>>> + * of this software and associated documentation files (the "Software"), to deal
> >>>> + * in the Software without restriction, including without limitation the rights
> >>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> >>>> + * copies of the Software, and to permit persons to whom the Software is
> >>>> + * furnished to do so, subject to the following conditions:
> >>>> + *
> >>>> + * The above copyright notice and this permission notice shall be included in
> >>>> + * all copies or substantial portions of the Software.
> >>>> + *
> >>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> >>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> >>>> + * THE SOFTWARE.
> >>>> + */
> >>>> +
> >>>> +#include "config-host.h"
> >>>> +
> >>>> +#include <poll.h>
> >>>> +#include "qemu-common.h"
> >>>> +#include "qemu/config-file.h"
> >>>> +#include "qemu/error-report.h"
> >>>> +#include "block/block_int.h"
> >>>> +#include "trace.h"
> >>>> +#include "qemu/iov.h"
> >>>> +#include "qemu/uri.h"
> >>>> +#include "sysemu/sysemu.h"
> >>>> +#include <nfsc/libnfs.h>
> >>>> +
> >>>> +typedef struct NFSClient {
> >>>> + struct nfs_context *context;
> >>>> + struct nfsfh *fh;
> >>>> + int events;
> >>>> + bool has_zero_init;
> >>>> +} NFSClient;
> >>>> +
> >>>> +typedef struct NFSRPC {
> >>>> + int status;
> >>>> + int complete;
> >>>> + QEMUIOVector *iov;
> >>>> + struct stat *st;
> >>>> + Coroutine *co;
> >>>> + QEMUBH *bh;
> >>>> +} NFSRPC;
> >>>> +
> >>>> +static void nfs_process_read(void *arg);
> >>>> +static void nfs_process_write(void *arg);
> >>>> +
> >>>> +static void nfs_set_events(NFSClient *client)
> >>>> +{
> >>>> + int ev = nfs_which_events(client->context);
> >>>> + if (ev != client->events) {
> >>>> + qemu_aio_set_fd_handler(nfs_get_fd(client->context),
> >>>> + (ev & POLLIN) ? nfs_process_read : NULL,
> >>>> + (ev & POLLOUT) ? nfs_process_write : NULL,
> >>>> + client);
> >>>> +
> >>>> + }
> >>>> + client->events = ev;
> >>>> +}
> >>>> +
> >>>> +static void nfs_process_read(void *arg)
> >>>> +{
> >>>> + NFSClient *client = arg;
> >>>> + nfs_service(client->context, POLLIN);
> >>>> + nfs_set_events(client);
> >>>> +}
> >>>> +
> >>>> +static void nfs_process_write(void *arg)
> >>>> +{
> >>>> + NFSClient *client = arg;
> >>>> + nfs_service(client->context, POLLOUT);
> >>>> + nfs_set_events(client);
> >>>> +}
> >>>> +
> >>>> +static void nfs_co_init_task(NFSClient *client, NFSRPC *task)
> >>>> +{
> >>>> + *task = (NFSRPC) {
> >>>> + .co = qemu_coroutine_self(),
> >>>> + };
> >>>> +}
> >>>> +
> >>>> +static void nfs_co_generic_bh_cb(void *opaque)
> >>>> +{
> >>>> + NFSRPC *task = opaque;
> >>>> + qemu_bh_delete(task->bh);
> >>>> + qemu_coroutine_enter(task->co, NULL);
> >>>> +}
> >>>> +
> >>>> +static void
> >>>> +nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
> >>>> + void *private_data)
> >>>> +{
> >>>> + NFSRPC *task = private_data;
> >>>> + task->complete = 1;
> >>>> + task->status = status;
> >>>> + if (task->status > 0 && task->iov) {
> >>>> + if (task->status <= task->iov->size) {
> >>> It feel very odd to compare something named status with something named size.
> >>>
> >>>> + qemu_iovec_from_buf(task->iov, 0, data, task->status);
> >>>> + } else {
> >>>> + task->status = -EIO;
> >>>> + }
> >>>> + }
> >>>> + if (task->status == 0 && task->st) {
> >>>> + memcpy(task->st, data, sizeof(struct stat));
> >>>> + }
> >>>> + if (task->co) {
> >>>> + task->bh = qemu_bh_new(nfs_co_generic_bh_cb, task);
> >>>> + qemu_bh_schedule(task->bh);
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +static int coroutine_fn nfs_co_readv(BlockDriverState *bs,
> >>>> + int64_t sector_num, int nb_sectors,
> >>>> + QEMUIOVector *iov)
> >>>> +{
> >>>> + NFSClient *client = bs->opaque;
> >>>> + NFSRPC task;
> >>>> +
> >>>> + nfs_co_init_task(client, &task);
> >>>> + task.iov = iov;
> >>>> +
> >>>> + if (nfs_pread_async(client->context, client->fh,
> >>>> + sector_num * BDRV_SECTOR_SIZE,
> >>>> + nb_sectors * BDRV_SECTOR_SIZE,
> >>>> + nfs_co_generic_cb, &task) != 0) {
> >>>> + return -ENOMEM;
> >>>> + }
> >>>> +
> >>>> + while (!task.complete) {
> >>>> + nfs_set_events(client);
> >>>> + qemu_coroutine_yield();
> >>>> + }
> >>>> +
> >>>> + if (task.status < 0) {
> >>>> + return task.status;
> >>>> + }
> >>>> +
> >>>> + /* zero pad short reads */
> >>>> + if (task.status < iov->size) {
> >>>> + qemu_iovec_memset(iov, task.status, 0, iov->size - task.status);
> >>>> + }
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
> >>>> + int64_t sector_num, int nb_sectors,
> >>>> + QEMUIOVector *iov)
> >>>> +{
> >>>> + NFSClient *client = bs->opaque;
> >>>> + NFSRPC task;
> >>>> + char *buf = NULL;
> >>>> +
> >>>> + nfs_co_init_task(client, &task);
> >>>> +
> >>>> + buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
> >>>> + qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
> >>>> +
> >>>> + if (nfs_pwrite_async(client->context, client->fh,
> >>>> + sector_num * BDRV_SECTOR_SIZE,
> >>>> + nb_sectors * BDRV_SECTOR_SIZE,
> >>>> + buf, nfs_co_generic_cb, &task) != 0) {
> >>>> + g_free(buf);
> >>>> + return -ENOMEM;
> >>>> + }
> >>>> +
> >>>> + while (!task.complete) {
> >>>> + nfs_set_events(client);
> >>>> + qemu_coroutine_yield();
> >>>> + }
> >>>> +
> >>>> + g_free(buf);
> >>>> +
> >>>> + if (task.status != nb_sectors * BDRV_SECTOR_SIZE) {
> >>>> + return task.status < 0 ? task.status : -EIO;
> >>>> + }
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
> >>>> +{
> >>>> + NFSClient *client = bs->opaque;
> >>>> + NFSRPC task;
> >>>> +
> >>>> + nfs_co_init_task(client, &task);
> >>>> +
> >>>> + if (nfs_fsync_async(client->context, client->fh, nfs_co_generic_cb,
> >>>> + &task) != 0) {
> >>>> + return -ENOMEM;
> >>>> + }
> >>>> +
> >>>> + while (!task.complete) {
> >>>> + nfs_set_events(client);
> >>>> + qemu_coroutine_yield();
> >>>> + }
> >>>> +
> >>>> + return task.status;
> >>>> +}
> >>>> +
> >>>> +/* TODO Convert to fine grained options */
> >>>> +static QemuOptsList runtime_opts = {
> >>>> + .name = "nfs",
> >>>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> >>>> + .desc = {
> >>>> + {
> >>>> + .name = "filename",
> >>>> + .type = QEMU_OPT_STRING,
> >>>> + .help = "URL to the NFS file",
> >>>> + },
> >>>> + { /* end of list */ }
> >>>> + },
> >>>> +};
> >>>> +
> >>>> +static void nfs_client_close(NFSClient *client)
> >>>> +{
> >>>> + if (client->context) {
> >>>> + if (client->fh) {
> >>>> + nfs_close(client->context, client->fh);
> >>>> + }
> >>>> + qemu_aio_set_fd_handler(nfs_get_fd(client->context), NULL, NULL, NULL);
> >>>> + nfs_destroy_context(client->context);
> >>>> + }
> >>>> + memset(client, 0, sizeof(NFSClient));
> >>>> +}
> >>>> +
> >>>> +static void nfs_file_close(BlockDriverState *bs)
> >>>> +{
> >>>> + NFSClient *client = bs->opaque;
> >>>> + nfs_client_close(client);
> >>>> +}
> >>>> +
> >>>> +static int64_t nfs_.cilclient_open(NFSClient *client, const char *filename,
> >>>> + int flags, Error **errp)
> >>>> +{
> >>>> + int ret = -EINVAL, i;
> >>>> + 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");
> >>>> + goto fail;
> >>> Should this be goto out since we don't have done nfs_init_context yet ?
> >>>> + }
> >>>> + strp = strrchr(uri->path, '/');
> >>>> + if (strp == NULL) {
> >>>> + error_setg(errp, "Invalid URL specified");
> >>>> + goto fail;
> >>> goto out ?
> >>>> + }
> >>>> + file = g_strdup(strp);
> >>>> + *strp = 0;
> >>>> +
> >>>> + client->context = nfs_init_context();
> >>>> + if (client->context == NULL) {
> >>>> + error_setg(errp, "Failed to init NFS context");
> >>>> + goto fail;
> >>>> + }
> >>>> +
> >>>> + qp = query_params_parse(uri->query);
> >>>> + 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 fail;
> >>>> + }
> >>>> + if (!strncmp(qp->p[i].name, "uid", 3)) {
> >>>> + nfs_set_uid(client->context, atoi(qp->p[i].value));
> >>>> + } else if (!strncmp(qp->p[i].name, "gid", 3)) {
> >>>> + nfs_set_gid(client->context, atoi(qp->p[i].value));
> >>>> + } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
> >>>> + nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
> >>>> + } else {
> >>>> + error_setg(errp, "Unknown NFS parameter name: %s",
> >>>> + qp->p[i].name);
> >>>> + goto fail;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + ret = nfs_mount(client->context, uri->server, uri->path);
> >>>> + if (ret < 0) {
> >>>> + error_setg(errp, "Failed to mount nfs share: %s",
> >>>> + nfs_get_error(client->context));
> >>>> + goto fail;
> >>>> + }
> >>>> +
> >>>> + if (flags & O_CREAT) {
> >>>> + ret = nfs_creat(client->context, file, 0600, &client->fh);
> >>>> + if (ret < 0) {
> >>>> + error_setg(errp, "Failed to create file: %s",
> >>>> + nfs_get_error(client->context));
> >>>> + goto fail;
> >>>> + }
> >>>> + } else {
> >>>> + ret = nfs_open(client->context, file, flags, &client->fh);
> >>>> + if (ret < 0) {
> >>>> + error_setg(errp, "Failed to open file : %s",
> >>>> + nfs_get_error(client->context));
> >>>> + goto fail;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + ret = nfs_fstat(client->context, client->fh, &st);
> >>>> + if (ret < 0) {
> >>>> + error_setg(errp, "Failed to fstat file: %s",
> >>>> + nfs_get_error(client->context));
> >>>> + goto fail;
> >>>> + }
> >>>> +
> >>>> + ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
> >>>> + 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);
> >>>> + g_free(file);
> >>>> + return ret;
> >>>> +}
> >>>> +
> >>>> +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;
> >>>> +
> >>>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> >>>> + qemu_opts_absorb_qdict(opts, options, &local_err);
> >>>> + if (error_is_set(&local_err)) {
> >>>> + qerror_report_err(local_err);
> >>> I have seen more usage of error_propagate(errp, local_err); in QEMU code.
> >>> Maybe I am missing the point.
> >>>
> >>>> + error_free(local_err);
> >>>> + return -EINVAL;
> >>>> + }
> >>>> + ret = nfs_client_open(client, qemu_opt_get(opts, "filename"),
> >>>> + (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
> >>>> + errp);
> >>>> + if (ret < 0) {
> >>>> + return ret;
> >>>> + }
> >>>> + bs->total_sectors = ret;
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int nfs_file_create(const char *url, QEMUOptionParameter *options,
> >>>> + Error **errp)
> >>>> +{
> >>>> + int ret = 0;
> >>>> + int64_t total_size = 0;
> >>>> + NFSClient *client = g_malloc0(sizeof(NFSClient));
> >>>> +
> >>>> + /* Read out options */
> >>>> + while (options && options->name) {
> >>>> + if (!strcmp(options->name, "size")) {
> >>>> + total_size = options->value.n;
> >>>> + }
> >>>> + options++;
> >>>> + }
> >>>> +
> >>>> + ret = nfs_client_open(client, url, O_CREAT, errp);
> >>>> + if (ret < 0) {
> >>>> + goto out;
> >>>> + }
> >>>> + ret = nfs_ftruncate(client->context, client->fh, total_size);
> >>>> +out:
> >>> Should the out: label be after nfs_client_close(client); since the code will
> >>> jump to it on nfs_client_open failure ?
> >> You are right, but it doesn't hurt since you can call nfs_client_close multiple
> >> times.
> >
> > Will you send another revision addressing the other comments?
>
> Sorry, which other comments are you referring to?
I replied to Benoit's email hilighting his questions which were not
addressed.
> Have you been able to test the patch without the bad commit?
Waiting for a final version to test.
Stefan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-30 14:23 ` Stefan Hajnoczi
@ 2014-01-30 21:33 ` Peter Lieven
0 siblings, 0 replies; 23+ messages in thread
From: Peter Lieven @ 2014-01-30 21:33 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Benoît Canet, Kevin Wolf, Fam Zheng, ronniesahlberg,
Jeff Cody, qemu-devel, Max Reitz, owasserm, Federico Simoncelli,
Stefan Hajnoczi, Wenchao Xia
Am 30.01.2014 um 15:23 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
> On Thu, Jan 30, 2014 at 10:12:35AM +0100, Peter Lieven wrote:
>>
>> Am 30.01.2014 um 10:05 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
>>
>>> On Wed, Jan 29, 2014 at 05:38:29PM +0100, Peter Lieven wrote:
>>>> On 29.01.2014 17:19, Benoît Canet wrote:
>>>>> Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
>>>>>> This patch adds native support for accessing images on NFS
>>>>>> shares without the requirement to actually mount the entire
>>>>>> NFS share on the host.
>>>>>>
>>>>>> NFS Images can simply be specified by an url of the form:
>>>>>> nfs://<host>/<export>/<filename>[?param=value[¶m2=value2[&...]]]
>>>>>>
>>>>>> For example:
>>>>>> qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2
>>>>>>
>>>>>> You need LibNFS from Ronnie Sahlberg available at:
>>>>>> git://github.com/sahlberg/libnfs.git
>>>>>> for this to work.
>>>>>>
>>>>>> During configure it is automatically probed for libnfs and support
>>>>>> is enabled on-the-fly. You can forbid or enforce libnfs support
>>>>>> with --disable-libnfs or --enable-libnfs respectively.
>>>>>>
>>>>>> Due to NFS restrictions you might need to execute your binaries
>>>>>> as root, allow them to open priviledged ports (<1024) or specify
>>>>>> insecure option on the NFS server.
>>>>>>
>>>>>> For additional information on ROOT vs. non-ROOT operation and URL
>>>>>> format + parameters see:
>>>>>> https://raw.github.com/sahlberg/libnfs/master/README
>>>>>>
>>>>>> Supported by qemu are the uid, gid and tcp-syncnt URL parameters.
>>>>>>
>>>>>> LibNFS currently support NFS version 3 only.
>>>>>>
>>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>>> ---
>>>>>> MAINTAINERS | 5 +
>>>>>> block/Makefile.objs | 1 +
>>>>>> block/nfs.c | 440 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> configure | 26 +++
>>>>>> qapi-schema.json | 1 +
>>>>>> 5 files changed, 473 insertions(+)
>>>>>> create mode 100644 block/nfs.c
>>>>>>
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>> index fb53242..f8411f9 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -936,6 +936,11 @@ M: Peter Lieven <pl@kamp.de>
>>>>>> S: Supported
>>>>>> F: block/iscsi.c
>>>>>> +NFS
>>>>>> +M: Peter Lieven <pl@kamp.de>
>>>>>> +S: Maintained
>>>>>> +F: block/nfs.c
>>>>>> +
>>>>>> SSH
>>>>>> M: Richard W.M. Jones <rjones@redhat.com>
>>>>>> S: Supported
>>>>>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>>>>>> index 4e8c91e..e254a21 100644
>>>>>> --- a/block/Makefile.objs
>>>>>> +++ b/block/Makefile.objs
>>>>>> @@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>>>>>> ifeq ($(CONFIG_POSIX),y)
>>>>>> block-obj-y += nbd.o nbd-client.o sheepdog.o
>>>>>> block-obj-$(CONFIG_LIBISCSI) += iscsi.o
>>>>>> +block-obj-$(CONFIG_LIBNFS) += nfs.o
>>>>>> block-obj-$(CONFIG_CURL) += curl.o
>>>>>> block-obj-$(CONFIG_RBD) += rbd.o
>>>>>> block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>>>>>> diff --git a/block/nfs.c b/block/nfs.c
>>>>>> new file mode 100644
>>>>>> index 0000000..2bbf7a2
>>>>>> --- /dev/null
>>>>>> +++ b/block/nfs.c
>>>>>> @@ -0,0 +1,440 @@
>>>>>> +/*
>>>>>> + * QEMU Block driver for native access to files on NFS shares
>>>>>> + *
>>>>>> + * Copyright (c) 2014 Peter Lieven <pl@kamp.de>
>>>>>> + *
>>>>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>>>> + * of this software and associated documentation files (the "Software"), to deal
>>>>>> + * in the Software without restriction, including without limitation the rights
>>>>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>>>>> + * copies of the Software, and to permit persons to whom the Software is
>>>>>> + * furnished to do so, subject to the following conditions:
>>>>>> + *
>>>>>> + * The above copyright notice and this permission notice shall be included in
>>>>>> + * all copies or substantial portions of the Software.
>>>>>> + *
>>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>>>> + * THE SOFTWARE.
>>>>>> + */
>>>>>> +
>>>>>> +#include "config-host.h"
>>>>>> +
>>>>>> +#include <poll.h>
>>>>>> +#include "qemu-common.h"
>>>>>> +#include "qemu/config-file.h"
>>>>>> +#include "qemu/error-report.h"
>>>>>> +#include "block/block_int.h"
>>>>>> +#include "trace.h"
>>>>>> +#include "qemu/iov.h"
>>>>>> +#include "qemu/uri.h"
>>>>>> +#include "sysemu/sysemu.h"
>>>>>> +#include <nfsc/libnfs.h>
>>>>>> +
>>>>>> +typedef struct NFSClient {
>>>>>> + struct nfs_context *context;
>>>>>> + struct nfsfh *fh;
>>>>>> + int events;
>>>>>> + bool has_zero_init;
>>>>>> +} NFSClient;
>>>>>> +
>>>>>> +typedef struct NFSRPC {
>>>>>> + int status;
>>>>>> + int complete;
>>>>>> + QEMUIOVector *iov;
>>>>>> + struct stat *st;
>>>>>> + Coroutine *co;
>>>>>> + QEMUBH *bh;
>>>>>> +} NFSRPC;
>>>>>> +
>>>>>> +static void nfs_process_read(void *arg);
>>>>>> +static void nfs_process_write(void *arg);
>>>>>> +
>>>>>> +static void nfs_set_events(NFSClient *client)
>>>>>> +{
>>>>>> + int ev = nfs_which_events(client->context);
>>>>>> + if (ev != client->events) {
>>>>>> + qemu_aio_set_fd_handler(nfs_get_fd(client->context),
>>>>>> + (ev & POLLIN) ? nfs_process_read : NULL,
>>>>>> + (ev & POLLOUT) ? nfs_process_write : NULL,
>>>>>> + client);
>>>>>> +
>>>>>> + }
>>>>>> + client->events = ev;
>>>>>> +}
>>>>>> +
>>>>>> +static void nfs_process_read(void *arg)
>>>>>> +{
>>>>>> + NFSClient *client = arg;
>>>>>> + nfs_service(client->context, POLLIN);
>>>>>> + nfs_set_events(client);
>>>>>> +}
>>>>>> +
>>>>>> +static void nfs_process_write(void *arg)
>>>>>> +{
>>>>>> + NFSClient *client = arg;
>>>>>> + nfs_service(client->context, POLLOUT);
>>>>>> + nfs_set_events(client);
>>>>>> +}
>>>>>> +
>>>>>> +static void nfs_co_init_task(NFSClient *client, NFSRPC *task)
>>>>>> +{
>>>>>> + *task = (NFSRPC) {
>>>>>> + .co = qemu_coroutine_self(),
>>>>>> + };
>>>>>> +}
>>>>>> +
>>>>>> +static void nfs_co_generic_bh_cb(void *opaque)
>>>>>> +{
>>>>>> + NFSRPC *task = opaque;
>>>>>> + qemu_bh_delete(task->bh);
>>>>>> + qemu_coroutine_enter(task->co, NULL);
>>>>>> +}
>>>>>> +
>>>>>> +static void
>>>>>> +nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
>>>>>> + void *private_data)
>>>>>> +{
>>>>>> + NFSRPC *task = private_data;
>>>>>> + task->complete = 1;
>>>>>> + task->status = status;
>>>>>> + if (task->status > 0 && task->iov) {
>>>>>> + if (task->status <= task->iov->size) {
>>>>> It feel very odd to compare something named status with something named size.
>>>>>
>>>>>> + qemu_iovec_from_buf(task->iov, 0, data, task->status);
>>>>>> + } else {
>>>>>> + task->status = -EIO;
>>>>>> + }
>>>>>> + }
>>>>>> + if (task->status == 0 && task->st) {
>>>>>> + memcpy(task->st, data, sizeof(struct stat));
>>>>>> + }
>>>>>> + if (task->co) {
>>>>>> + task->bh = qemu_bh_new(nfs_co_generic_bh_cb, task);
>>>>>> + qemu_bh_schedule(task->bh);
>>>>>> + }
>>>>>> +}
>>>>>> +
>>>>>> +static int coroutine_fn nfs_co_readv(BlockDriverState *bs,
>>>>>> + int64_t sector_num, int nb_sectors,
>>>>>> + QEMUIOVector *iov)
>>>>>> +{
>>>>>> + NFSClient *client = bs->opaque;
>>>>>> + NFSRPC task;
>>>>>> +
>>>>>> + nfs_co_init_task(client, &task);
>>>>>> + task.iov = iov;
>>>>>> +
>>>>>> + if (nfs_pread_async(client->context, client->fh,
>>>>>> + sector_num * BDRV_SECTOR_SIZE,
>>>>>> + nb_sectors * BDRV_SECTOR_SIZE,
>>>>>> + nfs_co_generic_cb, &task) != 0) {
>>>>>> + return -ENOMEM;
>>>>>> + }
>>>>>> +
>>>>>> + while (!task.complete) {
>>>>>> + nfs_set_events(client);
>>>>>> + qemu_coroutine_yield();
>>>>>> + }
>>>>>> +
>>>>>> + if (task.status < 0) {
>>>>>> + return task.status;
>>>>>> + }
>>>>>> +
>>>>>> + /* zero pad short reads */
>>>>>> + if (task.status < iov->size) {
>>>>>> + qemu_iovec_memset(iov, task.status, 0, iov->size - task.status);
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
>>>>>> + int64_t sector_num, int nb_sectors,
>>>>>> + QEMUIOVector *iov)
>>>>>> +{
>>>>>> + NFSClient *client = bs->opaque;
>>>>>> + NFSRPC task;
>>>>>> + char *buf = NULL;
>>>>>> +
>>>>>> + nfs_co_init_task(client, &task);
>>>>>> +
>>>>>> + buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
>>>>>> + qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
>>>>>> +
>>>>>> + if (nfs_pwrite_async(client->context, client->fh,
>>>>>> + sector_num * BDRV_SECTOR_SIZE,
>>>>>> + nb_sectors * BDRV_SECTOR_SIZE,
>>>>>> + buf, nfs_co_generic_cb, &task) != 0) {
>>>>>> + g_free(buf);
>>>>>> + return -ENOMEM;
>>>>>> + }
>>>>>> +
>>>>>> + while (!task.complete) {
>>>>>> + nfs_set_events(client);
>>>>>> + qemu_coroutine_yield();
>>>>>> + }
>>>>>> +
>>>>>> + g_free(buf);
>>>>>> +
>>>>>> + if (task.status != nb_sectors * BDRV_SECTOR_SIZE) {
>>>>>> + return task.status < 0 ? task.status : -EIO;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
>>>>>> +{
>>>>>> + NFSClient *client = bs->opaque;
>>>>>> + NFSRPC task;
>>>>>> +
>>>>>> + nfs_co_init_task(client, &task);
>>>>>> +
>>>>>> + if (nfs_fsync_async(client->context, client->fh, nfs_co_generic_cb,
>>>>>> + &task) != 0) {
>>>>>> + return -ENOMEM;
>>>>>> + }
>>>>>> +
>>>>>> + while (!task.complete) {
>>>>>> + nfs_set_events(client);
>>>>>> + qemu_coroutine_yield();
>>>>>> + }
>>>>>> +
>>>>>> + return task.status;
>>>>>> +}
>>>>>> +
>>>>>> +/* TODO Convert to fine grained options */
>>>>>> +static QemuOptsList runtime_opts = {
>>>>>> + .name = "nfs",
>>>>>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>>>>>> + .desc = {
>>>>>> + {
>>>>>> + .name = "filename",
>>>>>> + .type = QEMU_OPT_STRING,
>>>>>> + .help = "URL to the NFS file",
>>>>>> + },
>>>>>> + { /* end of list */ }
>>>>>> + },
>>>>>> +};
>>>>>> +
>>>>>> +static void nfs_client_close(NFSClient *client)
>>>>>> +{
>>>>>> + if (client->context) {
>>>>>> + if (client->fh) {
>>>>>> + nfs_close(client->context, client->fh);
>>>>>> + }
>>>>>> + qemu_aio_set_fd_handler(nfs_get_fd(client->context), NULL, NULL, NULL);
>>>>>> + nfs_destroy_context(client->context);
>>>>>> + }
>>>>>> + memset(client, 0, sizeof(NFSClient));
>>>>>> +}
>>>>>> +
>>>>>> +static void nfs_file_close(BlockDriverState *bs)
>>>>>> +{
>>>>>> + NFSClient *client = bs->opaque;
>>>>>> + nfs_client_close(client);
>>>>>> +}
>>>>>> +
>>>>>> +static int64_t nfs_.cilclient_open(NFSClient *client, const char *filename,
>>>>>> + int flags, Error **errp)
>>>>>> +{
>>>>>> + int ret = -EINVAL, i;
>>>>>> + 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");
>>>>>> + goto fail;
>>>>> Should this be goto out since we don't have done nfs_init_context yet ?
>>>>>> + }
>>>>>> + strp = strrchr(uri->path, '/');
>>>>>> + if (strp == NULL) {
>>>>>> + error_setg(errp, "Invalid URL specified");
>>>>>> + goto fail;
>>>>> goto out ?
>>>>>> + }
>>>>>> + file = g_strdup(strp);
>>>>>> + *strp = 0;
>>>>>> +
>>>>>> + client->context = nfs_init_context();
>>>>>> + if (client->context == NULL) {
>>>>>> + error_setg(errp, "Failed to init NFS context");
>>>>>> + goto fail;
>>>>>> + }
>>>>>> +
>>>>>> + qp = query_params_parse(uri->query);
>>>>>> + 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 fail;
>>>>>> + }
>>>>>> + if (!strncmp(qp->p[i].name, "uid", 3)) {
>>>>>> + nfs_set_uid(client->context, atoi(qp->p[i].value));
>>>>>> + } else if (!strncmp(qp->p[i].name, "gid", 3)) {
>>>>>> + nfs_set_gid(client->context, atoi(qp->p[i].value));
>>>>>> + } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
>>>>>> + nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
>>>>>> + } else {
>>>>>> + error_setg(errp, "Unknown NFS parameter name: %s",
>>>>>> + qp->p[i].name);
>>>>>> + goto fail;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + ret = nfs_mount(client->context, uri->server, uri->path);
>>>>>> + if (ret < 0) {
>>>>>> + error_setg(errp, "Failed to mount nfs share: %s",
>>>>>> + nfs_get_error(client->context));
>>>>>> + goto fail;
>>>>>> + }
>>>>>> +
>>>>>> + if (flags & O_CREAT) {
>>>>>> + ret = nfs_creat(client->context, file, 0600, &client->fh);
>>>>>> + if (ret < 0) {
>>>>>> + error_setg(errp, "Failed to create file: %s",
>>>>>> + nfs_get_error(client->context));
>>>>>> + goto fail;
>>>>>> + }
>>>>>> + } else {
>>>>>> + ret = nfs_open(client->context, file, flags, &client->fh);
>>>>>> + if (ret < 0) {
>>>>>> + error_setg(errp, "Failed to open file : %s",
>>>>>> + nfs_get_error(client->context));
>>>>>> + goto fail;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + ret = nfs_fstat(client->context, client->fh, &st);
>>>>>> + if (ret < 0) {
>>>>>> + error_setg(errp, "Failed to fstat file: %s",
>>>>>> + nfs_get_error(client->context));
>>>>>> + goto fail;
>>>>>> + }
>>>>>> +
>>>>>> + ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
>>>>>> + 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);
>>>>>> + g_free(file);
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +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;
>>>>>> +
>>>>>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>>>>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>>>>>> + if (error_is_set(&local_err)) {
>>>>>> + qerror_report_err(local_err);
>>>>> I have seen more usage of error_propagate(errp, local_err); in QEMU code.
>>>>> Maybe I am missing the point.
>>>>>
>>>>>> + error_free(local_err);
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> + ret = nfs_client_open(client, qemu_opt_get(opts, "filename"),
>>>>>> + (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY,
>>>>>> + errp);
>>>>>> + if (ret < 0) {
>>>>>> + return ret;
>>>>>> + }
>>>>>> + bs->total_sectors = ret;
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int nfs_file_create(const char *url, QEMUOptionParameter *options,
>>>>>> + Error **errp)
>>>>>> +{
>>>>>> + int ret = 0;
>>>>>> + int64_t total_size = 0;
>>>>>> + NFSClient *client = g_malloc0(sizeof(NFSClient));
>>>>>> +
>>>>>> + /* Read out options */
>>>>>> + while (options && options->name) {
>>>>>> + if (!strcmp(options->name, "size")) {
>>>>>> + total_size = options->value.n;
>>>>>> + }
>>>>>> + options++;
>>>>>> + }
>>>>>> +
>>>>>> + ret = nfs_client_open(client, url, O_CREAT, errp);
>>>>>> + if (ret < 0) {
>>>>>> + goto out;
>>>>>> + }
>>>>>> + ret = nfs_ftruncate(client->context, client->fh, total_size);
>>>>>> +out:
>>>>> Should the out: label be after nfs_client_close(client); since the code will
>>>>> jump to it on nfs_client_open failure ?
>>>> You are right, but it doesn't hurt since you can call nfs_client_close multiple
>>>> times.
>>>
>>> Will you send another revision addressing the other comments?
>>
>> Sorry, which other comments are you referring to?
>
> I replied to Benoit's email hilighting his questions which were not
> addressed.
Sorry, I missed the other comments.
>
>> Have you been able to test the patch without the bad commit?
>
> Waiting for a final version to test.
I cannot do that, unfortunately.
Ronnie, can you revert the bad commit and leave it for the 2.0 development.
It would be good if you bump the version to 1.9.2 so I can check
for that in the configure script
Peter
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-30 14:22 ` Stefan Hajnoczi
@ 2014-01-30 21:35 ` Peter Lieven
2014-01-31 8:57 ` Stefan Hajnoczi
0 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2014-01-30 21:35 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Benoît Canet, Kevin Wolf, Fam Zheng, ronniesahlberg,
Jeff Cody, qemu-devel, Max Reitz, owasserm, Federico Simoncelli,
Stefan Hajnoczi, Wenchao Xia
Am 30.01.2014 um 15:22 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
> On Wed, Jan 29, 2014 at 05:19:59PM +0100, Benoît Canet wrote:
>> Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
>
> Hi Peter,
> If I read your reply to Benoit correctly, you only addressed the
> questions about nfs_client_close(). Here are Benoits other comments and
> my thoughts on them:
>
>>> +static void
>>> +nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
>>> + void *private_data)
>>> +{
>>> + NFSRPC *task = private_data;
>>> + task->complete = 1;
>>> + task->status = status;
>>> + if (task->status > 0 && task->iov) {
>>> + if (task->status <= task->iov->size) {
>> It feel very odd to compare something named status with something named size.
>
> Maybe the argument name isn't ideal. Something like 'ret' or 'result'
> would be more commonplace.
I took the "status" from libnfs examples. I can change it in the qemu code if
you like.
>
> Not critical but would be nice to change.
>
>>> +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;
>>> +
>>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>>> + if (error_is_set(&local_err)) {
>>> + qerror_report_err(local_err);
>> I have seen more usage of error_propagate(errp, local_err); in QEMU code.
>> Maybe I am missing the point.
>
> Yes, I think you are right. The Error should be propagated to the
> caller. It's not clear to me whether we can ever get an error from
> qemu_opts_absorb_qdict() in this call site though.
Is there any action I should take here? If yes, can you advise what
to do please.
Peter
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-30 21:35 ` Peter Lieven
@ 2014-01-31 8:57 ` Stefan Hajnoczi
2014-01-31 9:11 ` Peter Lieven
0 siblings, 1 reply; 23+ messages in thread
From: Stefan Hajnoczi @ 2014-01-31 8:57 UTC (permalink / raw)
To: Peter Lieven
Cc: Benoît Canet, Kevin Wolf, Fam Zheng, ronnie sahlberg,
Jeff Cody, qemu-devel, Max Reitz, Orit Wasserman,
Federico Simoncelli, Stefan Hajnoczi, Wenchao Xia
On Thu, Jan 30, 2014 at 10:35 PM, Peter Lieven <pl@kamp.de> wrote:
>
> Am 30.01.2014 um 15:22 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
>
>> On Wed, Jan 29, 2014 at 05:19:59PM +0100, Benoît Canet wrote:
>>> Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
>>>> +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;
>>>> +
>>>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>>>> + if (error_is_set(&local_err)) {
>>>> + qerror_report_err(local_err);
>>> I have seen more usage of error_propagate(errp, local_err); in QEMU code.
>>> Maybe I am missing the point.
>>
>> Yes, I think you are right. The Error should be propagated to the
>> caller. It's not clear to me whether we can ever get an error from
>> qemu_opts_absorb_qdict() in this call site though.
>
> Is there any action I should take here? If yes, can you advise what
> to do please.
The issue is that nfs_file_open() takes an Error **errp argument.
This means the function should report detailed errors using the Error
object.
The patch prints and then discards the local_error instead of
propagating it to the caller's errp.
We should just propagate the error instead of printing it:
if (error_is_set(&local_err)) {
error_propagate(errp, local_err);
goto ...;
}
Stefan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-31 8:57 ` Stefan Hajnoczi
@ 2014-01-31 9:11 ` Peter Lieven
2014-01-31 10:46 ` Stefan Hajnoczi
0 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2014-01-31 9:11 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Benoît Canet, Kevin Wolf, Fam Zheng, ronnie sahlberg,
Jeff Cody, qemu-devel, Max Reitz, Orit Wasserman,
Federico Simoncelli, Stefan Hajnoczi, Wenchao Xia
On 31.01.2014 09:57, Stefan Hajnoczi wrote:
> On Thu, Jan 30, 2014 at 10:35 PM, Peter Lieven <pl@kamp.de> wrote:
>> Am 30.01.2014 um 15:22 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
>>
>>> On Wed, Jan 29, 2014 at 05:19:59PM +0100, Benoît Canet wrote:
>>>> Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
>>>>> +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;
>>>>> +
>>>>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>>>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>>>>> + if (error_is_set(&local_err)) {
>>>>> + qerror_report_err(local_err);
>>>> I have seen more usage of error_propagate(errp, local_err); in QEMU code.
>>>> Maybe I am missing the point.
>>> Yes, I think you are right. The Error should be propagated to the
>>> caller. It's not clear to me whether we can ever get an error from
>>> qemu_opts_absorb_qdict() in this call site though.
>> Is there any action I should take here? If yes, can you advise what
>> to do please.
> The issue is that nfs_file_open() takes an Error **errp argument.
> This means the function should report detailed errors using the Error
> object.
>
> The patch prints and then discards the local_error instead of
> propagating it to the caller's errp.
>
> We should just propagate the error instead of printing it:
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> goto ...;
Ok, you are just referring to this part in nfs_file_open:
if (error_is_set(&local_err)) {
qerror_report_err(local_err);
error_free(local_err);
return -EINVAL;
}
which I would change to:
if (error_is_set(&local_err)) {
error_propagate(errp, local_err);
return -EINVAL;
}
The use of error_setg in nfs_client_open is ok?
Peter
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-31 9:11 ` Peter Lieven
@ 2014-01-31 10:46 ` Stefan Hajnoczi
2014-01-31 11:16 ` Peter Lieven
0 siblings, 1 reply; 23+ messages in thread
From: Stefan Hajnoczi @ 2014-01-31 10:46 UTC (permalink / raw)
To: Peter Lieven
Cc: Benoît Canet, Kevin Wolf, Fam Zheng, ronnie sahlberg,
Jeff Cody, qemu-devel, Max Reitz, Orit Wasserman,
Federico Simoncelli, Stefan Hajnoczi, Wenchao Xia
On Fri, Jan 31, 2014 at 10:11 AM, Peter Lieven <pl@kamp.de> wrote:
> On 31.01.2014 09:57, Stefan Hajnoczi wrote:
>>
>> On Thu, Jan 30, 2014 at 10:35 PM, Peter Lieven <pl@kamp.de> wrote:
>>>
>>> Am 30.01.2014 um 15:22 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
>>>
>>>> On Wed, Jan 29, 2014 at 05:19:59PM +0100, Benoīt Canet wrote:
>>>>>
>>>>> Le Wednesday 29 Jan 2014 ą 09:50:21 (+0100), Peter Lieven a écrit :
>>>>>>
>>>>>> +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;
>>>>>> +
>>>>>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>>>>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>>>>>> + if (error_is_set(&local_err)) {
>>>>>> + qerror_report_err(local_err);
>>>>>
>>>>> I have seen more usage of error_propagate(errp, local_err); in QEMU
>>>>> code.
>>>>> Maybe I am missing the point.
>>>>
>>>> Yes, I think you are right. The Error should be propagated to the
>>>> caller. It's not clear to me whether we can ever get an error from
>>>> qemu_opts_absorb_qdict() in this call site though.
>>>
>>> Is there any action I should take here? If yes, can you advise what
>>> to do please.
>>
>> The issue is that nfs_file_open() takes an Error **errp argument.
>> This means the function should report detailed errors using the Error
>> object.
>>
>> The patch prints and then discards the local_error instead of
>> propagating it to the caller's errp.
>>
>> We should just propagate the error instead of printing it:
>> if (error_is_set(&local_err)) {
>> error_propagate(errp, local_err);
>> goto ...;
>
>
> Ok, you are just referring to this part in nfs_file_open:
>
> if (error_is_set(&local_err)) {
> qerror_report_err(local_err);
> error_free(local_err);
> return -EINVAL;
> }
>
> which I would change to:
>
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> return -EINVAL;
> }
Yes.
> The use of error_setg in nfs_client_open is ok?
Yes, it's fine.
The Error API is not 100% obvious when you first see it, but once you
learn the conventions it's pretty usable:
Functions take an Error **errp argument. This argument is optional,
so a caller that doesn't care about detailed error messages may pass
NULL. This has implications...
error_setg(errp, fmt, ...) handles errp == NULL internally so you can
call it unconditionally.
The tricky thing is that error_is_set() only works if errp is non-NULL
(otherwise error_setg() skips creating an Error object). So it means
you cannot rely on your errp argument and often functions will declare
Error *local_err = NULL, so they can pass &local_err to child
functions. Finally, error_propagate(errp, local_err) is used to pass
out the Error object to our caller.
Hope this summary makes the Error API clear.
Stefan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-31 10:46 ` Stefan Hajnoczi
@ 2014-01-31 11:16 ` Peter Lieven
0 siblings, 0 replies; 23+ messages in thread
From: Peter Lieven @ 2014-01-31 11:16 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Benoît Canet, Kevin Wolf, Fam Zheng, ronnie sahlberg,
Jeff Cody, qemu-devel, Max Reitz, Orit Wasserman,
Federico Simoncelli, Stefan Hajnoczi, Wenchao Xia
On 31.01.2014 11:46, Stefan Hajnoczi wrote:
> On Fri, Jan 31, 2014 at 10:11 AM, Peter Lieven <pl@kamp.de> wrote:
>> On 31.01.2014 09:57, Stefan Hajnoczi wrote:
>>> On Thu, Jan 30, 2014 at 10:35 PM, Peter Lieven <pl@kamp.de> wrote:
>>>> Am 30.01.2014 um 15:22 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
>>>>
>>>>> On Wed, Jan 29, 2014 at 05:19:59PM +0100, Benoīt Canet wrote:
>>>>>> Le Wednesday 29 Jan 2014 ą 09:50:21 (+0100), Peter Lieven a écrit :
>>>>>>> +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;
>>>>>>> +
>>>>>>> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>>>>>>> + qemu_opts_absorb_qdict(opts, options, &local_err);
>>>>>>> + if (error_is_set(&local_err)) {
>>>>>>> + qerror_report_err(local_err);
>>>>>> I have seen more usage of error_propagate(errp, local_err); in QEMU
>>>>>> code.
>>>>>> Maybe I am missing the point.
>>>>> Yes, I think you are right. The Error should be propagated to the
>>>>> caller. It's not clear to me whether we can ever get an error from
>>>>> qemu_opts_absorb_qdict() in this call site though.
>>>> Is there any action I should take here? If yes, can you advise what
>>>> to do please.
>>> The issue is that nfs_file_open() takes an Error **errp argument.
>>> This means the function should report detailed errors using the Error
>>> object.
>>>
>>> The patch prints and then discards the local_error instead of
>>> propagating it to the caller's errp.
>>>
>>> We should just propagate the error instead of printing it:
>>> if (error_is_set(&local_err)) {
>>> error_propagate(errp, local_err);
>>> goto ...;
>>
>> Ok, you are just referring to this part in nfs_file_open:
>>
>> if (error_is_set(&local_err)) {
>> qerror_report_err(local_err);
>> error_free(local_err);
>> return -EINVAL;
>> }
>>
>> which I would change to:
>>
>> if (error_is_set(&local_err)) {
>> error_propagate(errp, local_err);
>> return -EINVAL;
>> }
> Yes.
>
>> The use of error_setg in nfs_client_open is ok?
> Yes, it's fine.
>
> The Error API is not 100% obvious when you first see it, but once you
> learn the conventions it's pretty usable:
>
> Functions take an Error **errp argument. This argument is optional,
> so a caller that doesn't care about detailed error messages may pass
> NULL. This has implications...
>
> error_setg(errp, fmt, ...) handles errp == NULL internally so you can
> call it unconditionally.
>
> The tricky thing is that error_is_set() only works if errp is non-NULL
> (otherwise error_setg() skips creating an Error object). So it means
> you cannot rely on your errp argument and often functions will declare
> Error *local_err = NULL, so they can pass &local_err to child
> functions. Finally, error_propagate(errp, local_err) is used to pass
> out the Error object to our caller.
>
> Hope this summary makes the Error API clear.
>
> Stefan
Thanks for the explanation. I will sent you an updated series shortly.
Peter
--
Mit freundlichen Grüßen
Peter Lieven
...........................................................
KAMP Netzwerkdienste GmbH
Vestische Str. 89-91 | 46117 Oberhausen
Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
pl@kamp.de | http://www.kamp.de
Geschäftsführer: Heiner Lante | Michael Lante
Amtsgericht Duisburg | HRB Nr. 12154
USt-Id-Nr.: DE 120607556
...........................................................
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
2014-01-29 16:19 ` Benoît Canet
2014-01-29 16:38 ` Peter Lieven
2014-01-30 14:22 ` Stefan Hajnoczi
@ 2014-01-31 11:36 ` Peter Lieven
2 siblings, 0 replies; 23+ messages in thread
From: Peter Lieven @ 2014-01-31 11:36 UTC (permalink / raw)
To: Benoît Canet
Cc: Kevin Wolf, Fam Zheng, Stefan Hajnoczi, Jeff Cody, qemu-devel,
Max Reitz, owasserm, Federico Simoncelli, ronniesahlberg,
Wenchao Xia
On 29.01.2014 17:19, Benoît Canet wrote:
> Le Wednesday 29 Jan 2014 à 09:50:21 (+0100), Peter Lieven a écrit :
>> This patch adds native support for accessing images on NFS
>> shares without the requirement to actually mount the entire
>> NFS share on the host.
>>
>> NFS Images can simply be specified by an url of the form:
>> nfs://<host>/<export>/<filename>[?param=value[¶m2=value2[&...]]]
>>
>> For example:
>> qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2
>>
>> You need LibNFS from Ronnie Sahlberg available at:
>> git://github.com/sahlberg/libnfs.git
>> for this to work.
>>
>> During configure it is automatically probed for libnfs and support
>> is enabled on-the-fly. You can forbid or enforce libnfs support
>> with --disable-libnfs or --enable-libnfs respectively.
>>
>> Due to NFS restrictions you might need to execute your binaries
>> as root, allow them to open priviledged ports (<1024) or specify
>> insecure option on the NFS server.
>>
>> For additional information on ROOT vs. non-ROOT operation and URL
>> format + parameters see:
>> https://raw.github.com/sahlberg/libnfs/master/README
>>
>> Supported by qemu are the uid, gid and tcp-syncnt URL parameters.
>>
>> LibNFS currently support NFS version 3 only.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> MAINTAINERS | 5 +
>> block/Makefile.objs | 1 +
>> block/nfs.c | 440 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> configure | 26 +++
>> qapi-schema.json | 1 +
>> 5 files changed, 473 insertions(+)
>> create mode 100644 block/nfs.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index fb53242..f8411f9 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -936,6 +936,11 @@ M: Peter Lieven <pl@kamp.de>
>> S: Supported
>> F: block/iscsi.c
>>
>> +NFS
>> +M: Peter Lieven <pl@kamp.de>
>> +S: Maintained
>> +F: block/nfs.c
>> +
>> SSH
>> M: Richard W.M. Jones <rjones@redhat.com>
>> S: Supported
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index 4e8c91e..e254a21 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>> ifeq ($(CONFIG_POSIX),y)
>> block-obj-y += nbd.o nbd-client.o sheepdog.o
>> block-obj-$(CONFIG_LIBISCSI) += iscsi.o
>> +block-obj-$(CONFIG_LIBNFS) += nfs.o
>> block-obj-$(CONFIG_CURL) += curl.o
>> block-obj-$(CONFIG_RBD) += rbd.o
>> block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>> diff --git a/block/nfs.c b/block/nfs.c
>> new file mode 100644
>> index 0000000..2bbf7a2
>> --- /dev/null
>> +++ b/block/nfs.c
>> @@ -0,0 +1,440 @@
>> +/*
>> + * QEMU Block driver for native access to files on NFS shares
>> + *
>> + * Copyright (c) 2014 Peter Lieven <pl@kamp.de>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "config-host.h"
>> +
>> +#include <poll.h>
>> +#include "qemu-common.h"
>> +#include "qemu/config-file.h"
>> +#include "qemu/error-report.h"
>> +#include "block/block_int.h"
>> +#include "trace.h"
>> +#include "qemu/iov.h"
>> +#include "qemu/uri.h"
>> +#include "sysemu/sysemu.h"
>> +#include <nfsc/libnfs.h>
>> +
>> +typedef struct NFSClient {
>> + struct nfs_context *context;
>> + struct nfsfh *fh;
>> + int events;
>> + bool has_zero_init;
>> +} NFSClient;
>> +
>> +typedef struct NFSRPC {
>> + int status;
>> + int complete;
>> + QEMUIOVector *iov;
>> + struct stat *st;
>> + Coroutine *co;
>> + QEMUBH *bh;
>> +} NFSRPC;
>> +
>> +static void nfs_process_read(void *arg);
>> +static void nfs_process_write(void *arg);
>> +
>> +static void nfs_set_events(NFSClient *client)
>> +{
>> + int ev = nfs_which_events(client->context);
>> + if (ev != client->events) {
>> + qemu_aio_set_fd_handler(nfs_get_fd(client->context),
>> + (ev & POLLIN) ? nfs_process_read : NULL,
>> + (ev & POLLOUT) ? nfs_process_write : NULL,
>> + client);
>> +
>> + }
>> + client->events = ev;
>> +}
>> +
>> +static void nfs_process_read(void *arg)
>> +{
>> + NFSClient *client = arg;
>> + nfs_service(client->context, POLLIN);
>> + nfs_set_events(client);
>> +}
>> +
>> +static void nfs_process_write(void *arg)
>> +{
>> + NFSClient *client = arg;
>> + nfs_service(client->context, POLLOUT);
>> + nfs_set_events(client);
>> +}
>> +
>> +static void nfs_co_init_task(NFSClient *client, NFSRPC *task)
>> +{
>> + *task = (NFSRPC) {
>> + .co = qemu_coroutine_self(),
>> + };
>> +}
>> +
>> +static void nfs_co_generic_bh_cb(void *opaque)
>> +{
>> + NFSRPC *task = opaque;
>> + qemu_bh_delete(task->bh);
>> + qemu_coroutine_enter(task->co, NULL);
>> +}
>> +
>> +static void
>> +nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
>> + void *private_data)
>> +{
>> + NFSRPC *task = private_data;
>> + task->complete = 1;
>> + task->status = status;
>> + if (task->status > 0 && task->iov) {
>> + if (task->status <= task->iov->size) {
> It feel very odd to compare something named status with something named size.
>
>> + qemu_iovec_from_buf(task->iov, 0, data, task->status);
>> + } else {
>> + task->status = -EIO;
>> + }
>> + }
>> + if (task->status == 0 && task->st) {
>> + memcpy(task->st, data, sizeof(struct stat));
>> + }
>> + if (task->co) {
>> + task->bh = qemu_bh_new(nfs_co_generic_bh_cb, task);
>> + qemu_bh_schedule(task->bh);
>> + }
>> +}
>> +
>> +static int coroutine_fn nfs_co_readv(BlockDriverState *bs,
>> + int64_t sector_num, int nb_sectors,
>> + QEMUIOVector *iov)
>> +{
>> + NFSClient *client = bs->opaque;
>> + NFSRPC task;
>> +
>> + nfs_co_init_task(client, &task);
>> + task.iov = iov;
>> +
>> + if (nfs_pread_async(client->context, client->fh,
>> + sector_num * BDRV_SECTOR_SIZE,
>> + nb_sectors * BDRV_SECTOR_SIZE,
>> + nfs_co_generic_cb, &task) != 0) {
>> + return -ENOMEM;
>> + }
>> +
>> + while (!task.complete) {
>> + nfs_set_events(client);
>> + qemu_coroutine_yield();
>> + }
>> +
>> + if (task.status < 0) {
>> + return task.status;
>> + }
>> +
>> + /* zero pad short reads */
>> + if (task.status < iov->size) {
>> + qemu_iovec_memset(iov, task.status, 0, iov->size - task.status);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
>> + int64_t sector_num, int nb_sectors,
>> + QEMUIOVector *iov)
>> +{
>> + NFSClient *client = bs->opaque;
>> + NFSRPC task;
>> + char *buf = NULL;
>> +
>> + nfs_co_init_task(client, &task);
>> +
>> + buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
>> + qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
>> +
>> + if (nfs_pwrite_async(client->context, client->fh,
>> + sector_num * BDRV_SECTOR_SIZE,
>> + nb_sectors * BDRV_SECTOR_SIZE,
>> + buf, nfs_co_generic_cb, &task) != 0) {
>> + g_free(buf);
>> + return -ENOMEM;
>> + }
>> +
>> + while (!task.complete) {
>> + nfs_set_events(client);
>> + qemu_coroutine_yield();
>> + }
>> +
>> + g_free(buf);
>> +
>> + if (task.status != nb_sectors * BDRV_SECTOR_SIZE) {
>> + return task.status < 0 ? task.status : -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
>> +{
>> + NFSClient *client = bs->opaque;
>> + NFSRPC task;
>> +
>> + nfs_co_init_task(client, &task);
>> +
>> + if (nfs_fsync_async(client->context, client->fh, nfs_co_generic_cb,
>> + &task) != 0) {
>> + return -ENOMEM;
>> + }
>> +
>> + while (!task.complete) {
>> + nfs_set_events(client);
>> + qemu_coroutine_yield();
>> + }
>> +
>> + return task.status;
>> +}
>> +
>> +/* TODO Convert to fine grained options */
>> +static QemuOptsList runtime_opts = {
>> + .name = "nfs",
>> + .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>> + .desc = {
>> + {
>> + .name = "filename",
>> + .type = QEMU_OPT_STRING,
>> + .help = "URL to the NFS file",
>> + },
>> + { /* end of list */ }
>> + },
>> +};
>> +
>> +static void nfs_client_close(NFSClient *client)
>> +{
>> + if (client->context) {
>> + if (client->fh) {
>> + nfs_close(client->context, client->fh);
>> + }
>> + qemu_aio_set_fd_handler(nfs_get_fd(client->context), NULL, NULL, NULL);
>> + nfs_destroy_context(client->context);
>> + }
>> + memset(client, 0, sizeof(NFSClient));
>> +}
>> +
>> +static void nfs_file_close(BlockDriverState *bs)
>> +{
>> + NFSClient *client = bs->opaque;
>> + nfs_client_close(client);
>> +}
>> +
>> +static int64_t nfs_.cilclient_open(NFSClient *client, const char *filename,
>> + int flags, Error **errp)
>> +{
>> + int ret = -EINVAL, i;
>> + 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");
>> + goto fail;
> Should this be goto out since we don't have done nfs_init_context yet ?
it doesn't matter nfs_client_close can be called with client->context == NULL.
Sorry I entirely missed your other comments.
Peter
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2014-01-31 11:37 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-29 8:50 [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 1/5] " Peter Lieven
2014-01-29 16:19 ` Benoît Canet
2014-01-29 16:38 ` Peter Lieven
2014-01-30 9:05 ` Stefan Hajnoczi
2014-01-30 9:12 ` Peter Lieven
2014-01-30 14:23 ` Stefan Hajnoczi
2014-01-30 21:33 ` Peter Lieven
2014-01-30 14:22 ` Stefan Hajnoczi
2014-01-30 21:35 ` Peter Lieven
2014-01-31 8:57 ` Stefan Hajnoczi
2014-01-31 9:11 ` Peter Lieven
2014-01-31 10:46 ` Stefan Hajnoczi
2014-01-31 11:16 ` Peter Lieven
2014-01-31 11:36 ` Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 2/5] qemu-iotests: change _supported_proto to file for various tests Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 3/5] qemu-iotests: enable support for NFS protocol Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 4/5] qemu-iotests: enable test 016 and 025 to work with " Peter Lieven
2014-01-29 8:50 ` [Qemu-devel] [PATCHv7 5/5] qemu-iotests: blacklist test 020 for " Peter Lieven
2014-01-29 13:59 ` [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS Stefan Hajnoczi
2014-01-29 14:13 ` Peter Lieven
2014-01-29 14:22 ` Peter Lieven
2014-01-29 14:26 ` Peter Lieven
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.