Re: [Qemu-devel] [PATCHv7 1/5] block: add native support for NFS

[Peter Lieven]

Am 30.01.2014 um 15:23 schrieb Stefan Hajnoczi <address@hidden>:

> On Thu, Jan 30, 2014 at 10:12:35AM +0100, Peter Lieven wrote:
>> 
>> Am 30.01.2014 um 10:05 schrieb Stefan Hajnoczi <address@hidden>:
>> 
>>> 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[&param2=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?
>> 
>> 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