[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv2] block: add native support for NFS
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCHv2] block: add native support for NFS |
Date: |
Tue, 17 Dec 2013 17:47:30 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Tue, Dec 17, 2013 at 10:15:25AM +0100, Peter Lieven wrote:
> 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>
>
> 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.
>
> Signed-off-by: Peter Lieven <address@hidden>
> ---
> 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]
>
> MAINTAINERS | 5 +
> block/Makefile.objs | 1 +
> block/nfs.c | 419
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> configure | 38 +++++
> 4 files changed, 463 insertions(+)
> create mode 100644 block/nfs.c
Which NFS protocol versions are supported by current libnfs?
> +#include <poll.h>
Why is this header included?
> +typedef struct nfsclient {
Please either drop the struct tag or use "NFSClient".
> +static void
> +nfs_co_generic_cb(int status, struct nfs_context *nfs, void *data,
> + void *private_data)
> +{
> + NFSTask *Task = private_data;
lowercase "task" local variable name please.
> +static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
> + int64_t sector_num, int nb_sectors,
> + QEMUIOVector *iov)
> +{
> + NFSClient *client = bs->opaque;
> + NFSTask 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 -EIO;
Can we get a more detailed errno here? (e.g. ENOSPC)
> + }
> +
> + 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;
> + }
> +
> + bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors);
Why is this necessary? block.c will update bs->total_sectors if the
file is growable.
> + /* set to -ENOTSUP since bdrv_allocated_file_size is only used
> + * in qemu-img open. So we can use the cached value for allocate
> + * filesize obtained from fstat at open time */
> + client->allocated_file_size = -ENOTSUP;
Can you implement this fully? By stubbing it out like this we won't be
able to call get_allocated_file_size() at runtime in the future without
updating the nfs block driver code. It's just an fstat call, shouldn't
be too hard to implement properly :).
> + if (client->context == NULL) {
> + error_setg(errp, "Failed to init NFS context");
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + if (strlen(filename) <= 6) {
> + error_setg(errp, "Invalid server in URL");
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + server = g_strdup(filename + 6);
> + if (server[0] == '/' || server[0] == '\0') {
> + error_setg(errp, "Invalid server in URL");
> + ret = -EINVAL;
> + goto fail;
> + }
> + strp = strchr(server, '/');
> + if (strp == NULL) {
> + error_setg(errp, "Invalid URL specified.\n");
> + ret = -EINVAL;
> + goto fail;
> + }
> + path = g_strdup(strp);
> + *strp = 0;
> + strp = strrchr(path, '/');
> + if (strp == NULL) {
> + error_setg(errp, "Invalid URL specified.\n");
> + ret = -EINVAL;
> + goto fail;
> + }
> + file = g_strdup(strp);
> + *strp = 0;
Can you use util/uri.c to avoid the string manipulation? It can extract
the different parts for validation: scheme, server, port, and path.
> +static int nfs_file_create(const char *filename, QEMUOptionParameter
> *options,
> + Error **errp)
> +{
> + int ret = 0;
> + int64_t total_size = 0;
> + BlockDriverState *bs;
> + NFSClient *client = NULL;
> + QDict *bs_options;
> +
> + bs = bdrv_new("");
This approach seems a little risky to me. bs is a fake
BlockDriverState with many fields not initialized. bdrv_*() calls would
crash.
How about a static nfs_open() function that operates on NFSClient and is
shared by nfs_file_create() and nfs_file_open()?
> +##########################################
> +# Do we have libnfs
> +if test "$libnfs" != "no" ; then
> + cat > $TMPC << EOF
> +#include <nfsc/libnfs-zdr.h>
> +#include <nfsc/libnfs.h>
> +#include <nfsc/libnfs-raw.h>
> +#include <nfsc/libnfs-raw-mount.h>
> +int main(void) {
> + nfs_init_context();
> + nfs_pread_async(0,0,0,0,0,0);
> + nfs_pwrite_async(0,0,0,0,0,0,0);
> + nfs_fstat(0,0,0);
> + return 0;
> + }
> +EOF
> + if compile_prog "-Werror" "-lnfs" ; then
> + libnfs="yes"
> + LIBS="$LIBS -lnfs"
pkg-config is usually better than hardcoding names. Is pkg-config
available for libnfs?
- [Qemu-devel] [PATCHv2] block: add native support for NFS, Peter Lieven, 2013/12/17
- Re: [Qemu-devel] [PATCHv2] block: add native support for NFS,
Stefan Hajnoczi <=
- Re: [Qemu-devel] [PATCHv2] block: add native support for NFS, Peter Lieven, 2013/12/17
- Re: [Qemu-devel] [PATCHv2] block: add native support for NFS, ronnie sahlberg, 2013/12/17
- Re: [Qemu-devel] [PATCHv2] block: add native support for NFS, Peter Lieven, 2013/12/17
- Re: [Qemu-devel] [PATCHv2] block: add native support for NFS, Eric Blake, 2013/12/17
- Re: [Qemu-devel] [PATCHv2] block: add native support for NFS, ronnie sahlberg, 2013/12/17
- Re: [Qemu-devel] [PATCHv2] block: add native support for NFS, Peter Lieven, 2013/12/17
- Re: [Qemu-devel] [PATCHv2] block: add native support for NFS, ronnie sahlberg, 2013/12/17
- Re: [Qemu-devel] [PATCHv2] block: add native support for NFS, Peter Lieven, 2013/12/17
Re: [Qemu-devel] [PATCHv2] block: add native support for NFS, Peter Lieven, 2013/12/20