Re: [Qemu-devel] [PATCH] block: add native support for NFS

[Fam Zheng <famz@redhat.com>]

On 2013年12月17日 15:53, Peter Lieven wrote:
> Hi Fam,
>
> On 17.12.2013 05:07, Fam Zheng wrote:
>> On 2013年12月16日 23:34, Peter Lieven wrote:
>>> +static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
>>> +                                        int64_t sector_num, int
>>> nb_sectors,
>>> +                                        QEMUIOVector *iov)
>>> +{
>>> +    nfsclient *client = bs->opaque;
>>> +    struct 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;
>>> +    }
>>> +
>>> +    while (!Task.complete) {
>>> +        nfs_set_events(client);
>>> +        qemu_coroutine_yield();
>>> +    }
>>> +
>>> +    g_free(buf);
>>> +
>>> +    if (Task.status != nb_sectors * BDRV_SECTOR_SIZE) {
>>> +        return -EIO;
>>> +    }
>>> +
>>> +    bs->total_sectors = MAX(bs->total_sectors, sector_num +
>>> nb_sectors);
>>> +    client->allocated_file_size = -ENOTSUP;
>>
>> Why does allocated_file_size become not supported after a write?
> I thought that someone would ask this ;-) bdrv_allocated_file_size is only
> used in image info. I saved some code here implementing an async call.
> On open I fstat anyway and store that value. For qemu-img info this is
> sufficient, but the allocated size likely changes after a write. -ENOTSUP
> is the default if bdrv_allocated_file_size is not implemented.

OK. Please add some comment here.

>>> +static int nfs_file_open_common(BlockDriverState *bs, QDict
>>> *options, int flags,
>>> +                         int open_flags, Error **errp)
>>> +{
>>> +    nfsclient *client = bs->opaque;
>>> +    const char *filename;
>>> +    int ret = 0;
>>> +    QemuOpts *opts;
>>> +    Error *local_err = NULL;
>>> +    char *server = NULL, *path = NULL, *file = NULL, *strp;
>>> +    struct stat st;
>>> +
>>> +    opts = qemu_opts_create_nofail(&runtime_opts);
>>> +    qemu_opts_absorb_qdict(opts, options, &local_err);
>>> +    if (error_is_set(&local_err)) {
>>> +        qerror_report_err(local_err);
>>> +        error_free(local_err);
>>> +        ret = -EINVAL;
>>> +        goto fail;
>>> +    }
>>> +
>>> +    filename = qemu_opt_get(opts, "filename");
>>> +
>>> +    client->context = nfs_init_context();
>>> +
>>> +    if (client->context == NULL) {
>>> +        error_setg(errp, "Failed to init NFS context");
>>> +        ret = -EINVAL;
>>> +        goto fail;
>>> +    }
>>> +
>>> +    server = g_strdup(filename + 6);
>>
>> Please check the length of filename is longer than 6 before accessing
>> filename[6].
> Good point. I will check for this, but in fact I think it can't happen
> because we will
> never end up there if filename does not start with nfs://

True, at least for now, but it doesn't hurt to be defensive, especially 
with strings.

>>
>>> +    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;
>>> +
>>> +    if (nfs_mount(client->context, server, path) != 0) {
>>> +        error_setg(errp, "Failed to mount nfs share: %s",
>>> +                    nfs_get_error(client->context));
>>> +        ret = -EINVAL;
>>> +        goto fail;
>>> +    }
>>> +
>>> +    if (open_flags & O_CREAT) {
>>> +        if (nfs_creat(client->context, file, 0600, &client->fh) != 0) {
>>> +            error_setg(errp, "Failed to create file: %s",
>>> + nfs_get_error(client->context));
>>> +            ret = -EINVAL;
>>> +            goto fail;
>>> +        }
>>> +    } else {
>>> +        open_flags = (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY;
>>> +        if (nfs_open(client->context, file, open_flags, &client->fh)
>>> != 0) {
>>> +            error_setg(errp, "Failed to open file : %s",
>>> +                       nfs_get_error(client->context));
>>> +            ret = -EINVAL;
>>> +            goto fail;
>>> +        }
>>> +    }
>>> +
>>> +    if (nfs_fstat(client->context, client->fh, &st) != 0) {
>>> +        error_setg(errp, "Failed to fstat file: %s",
>>> +                   nfs_get_error(client->context));
>>> +        ret = -EIO;
>>> +        goto fail;
>>> +    }
>>> +
>>> +    bs->total_sectors = st.st_size / BDRV_SECTOR_SIZE;
>>
>> Please use DIV_ROUND_UP(). Otherwise the remainder in last sector
>> couldn't be read.
> Will do. Can't it happen that we end up reading unallocated sectors?

Hmm, maybe. Not missing last bytes of unaligned sector is essential for 
VMDK description file. But you are right, please add check code and make 
sure that we don't read beyond EOF as well.

> Thanks for reviewing!
>
> One wish as I think you are the VMDK format maintainer. Can you rework
> vmdk_create and possible
> other fucntions in VMDK to use only bdrv_* functions (like in
> qcow2_create). Currently its not possible to create
> a VMDK on an NFS share directly caused by useing qemu_file_* calls.
> This also affectecs other drivers. QCOW2 and RAW work perfectly.

Yes, this is a good request. I have this on my todo list, thanks for 
reminding.

Fam