[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS |
Date: |
Thu, 30 Jan 2014 10:05:55 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> >>---
> >> 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 <address@hidden>
> >> S: Supported
> >> F: block/iscsi.c
> >>+NFS
> >>+M: Peter Lieven <address@hidden>
> >>+S: Maintained
> >>+F: block/nfs.c
> >>+
> >> SSH
> >> M: Richard W.M. Jones <address@hidden>
> >> 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 <address@hidden>
> >>+ *
> >>+ * 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
- [Qemu-devel] [PATCHv7 0/5] block: add native support for NFS, Peter Lieven, 2014/01/29
- Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS, Stefan Hajnoczi, 2014/01/30
- Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS, Peter Lieven, 2014/01/30
- Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS, Stefan Hajnoczi, 2014/01/31
- Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS, Peter Lieven, 2014/01/31
- Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS, Stefan Hajnoczi, 2014/01/31
- Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS, Peter Lieven, 2014/01/31
- Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS, Peter Lieven, 2014/01/31