From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40018) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vsq20-0002vR-6m for qemu-devel@nongnu.org; Tue, 17 Dec 2013 03:29:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vsq1u-0003wP-41 for qemu-devel@nongnu.org; Tue, 17 Dec 2013 03:29:32 -0500 Received: from mx1.redhat.com ([209.132.183.28]:15739) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vsq1t-0003wB-Rj for qemu-devel@nongnu.org; Tue, 17 Dec 2013 03:29:26 -0500 Message-ID: <52B00B4F.9060406@redhat.com> Date: Tue, 17 Dec 2013 16:29:03 +0800 From: Fam Zheng MIME-Version: 1.0 References: <1387208069-9302-1-git-send-email-pl@kamp.de> <52AFCDF4.9020804@redhat.com> <52B002F2.4060906@kamp.de> In-Reply-To: <52B002F2.4060906@kamp.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] block: add native support for NFS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, stefanha@redhat.com, ronniesahlberg@gmail.com On 2013=E5=B9=B412=E6=9C=8817=E6=97=A5 15:53, Peter Lieven wrote: > Hi Fam, > > On 17.12.2013 05:07, Fam Zheng wrote: >> On 2013=E5=B9=B412=E6=9C=8816=E6=97=A5 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 =3D bs->opaque; >>> + struct NFSTask Task; >>> + char *buf =3D NULL; >>> + >>> + nfs_co_init_task(client, &Task); >>> + >>> + buf =3D 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) !=3D 0) { >>> + g_free(buf); >>> + return -EIO; >>> + } >>> + >>> + while (!Task.complete) { >>> + nfs_set_events(client); >>> + qemu_coroutine_yield(); >>> + } >>> + >>> + g_free(buf); >>> + >>> + if (Task.status !=3D nb_sectors * BDRV_SECTOR_SIZE) { >>> + return -EIO; >>> + } >>> + >>> + bs->total_sectors =3D MAX(bs->total_sectors, sector_num + >>> nb_sectors); >>> + client->allocated_file_size =3D -ENOTSUP; >> >> Why does allocated_file_size become not supported after a write? > I thought that someone would ask this ;-) bdrv_allocated_file_size is o= nly > 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. -ENOTS= UP > 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 =3D bs->opaque; >>> + const char *filename; >>> + int ret =3D 0; >>> + QemuOpts *opts; >>> + Error *local_err =3D NULL; >>> + char *server =3D NULL, *path =3D NULL, *file =3D NULL, *strp; >>> + struct stat st; >>> + >>> + opts =3D 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 =3D -EINVAL; >>> + goto fail; >>> + } >>> + >>> + filename =3D qemu_opt_get(opts, "filename"); >>> + >>> + client->context =3D nfs_init_context(); >>> + >>> + if (client->context =3D=3D NULL) { >>> + error_setg(errp, "Failed to init NFS context"); >>> + ret =3D -EINVAL; >>> + goto fail; >>> + } >>> + >>> + server =3D 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=20 with strings. >> >>> + if (server[0] =3D=3D '/' || server[0] =3D=3D '\0') { >>> + error_setg(errp, "Invalid server in URL"); >>> + ret =3D -EINVAL; >>> + goto fail; >>> + } >>> + strp =3D strchr(server, '/'); >>> + if (strp =3D=3D NULL) { >>> + error_setg(errp, "Invalid URL specified.\n"); >>> + ret =3D -EINVAL; >>> + goto fail; >>> + } >>> + path =3D g_strdup(strp); >>> + *strp =3D 0; >>> + strp =3D strrchr(path, '/'); >>> + if (strp =3D=3D NULL) { >>> + error_setg(errp, "Invalid URL specified.\n"); >>> + ret =3D -EINVAL; >>> + goto fail; >>> + } >>> + file =3D g_strdup(strp); >>> + *strp =3D 0; >>> + >>> + if (nfs_mount(client->context, server, path) !=3D 0) { >>> + error_setg(errp, "Failed to mount nfs share: %s", >>> + nfs_get_error(client->context)); >>> + ret =3D -EINVAL; >>> + goto fail; >>> + } >>> + >>> + if (open_flags & O_CREAT) { >>> + if (nfs_creat(client->context, file, 0600, &client->fh) !=3D= 0) { >>> + error_setg(errp, "Failed to create file: %s", >>> + nfs_get_error(client->context)); >>> + ret =3D -EINVAL; >>> + goto fail; >>> + } >>> + } else { >>> + open_flags =3D (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY; >>> + if (nfs_open(client->context, file, open_flags, &client->fh) >>> !=3D 0) { >>> + error_setg(errp, "Failed to open file : %s", >>> + nfs_get_error(client->context)); >>> + ret =3D -EINVAL; >>> + goto fail; >>> + } >>> + } >>> + >>> + if (nfs_fstat(client->context, client->fh, &st) !=3D 0) { >>> + error_setg(errp, "Failed to fstat file: %s", >>> + nfs_get_error(client->context)); >>> + ret =3D -EIO; >>> + goto fail; >>> + } >>> + >>> + bs->total_sectors =3D 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=20 VMDK description file. But you are right, please add check code and make=20 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=20 reminding. Fam