On Thu, Jan 30, 2014 at 10:12:35AM +0100, Peter Lieven wrote: > > Am 30.01.2014 um 10:05 schrieb Stefan Hajnoczi <stefa...@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 <p...@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 <p...@kamp.de> > >>>> S: Supported > >>>> F: block/iscsi.c > >>>> +NFS > >>>> +M: Peter Lieven <p...@kamp.de> > >>>> +S: Maintained > >>>> +F: block/nfs.c > >>>> + > >>>> SSH > >>>> M: Richard W.M. Jones <rjo...@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 <p...@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
