From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:59894) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMli8-0006ig-VY for qemu-devel@nongnu.org; Wed, 18 May 2011 14:43:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QMli7-0005Bs-O7 for qemu-devel@nongnu.org; Wed, 18 May 2011 14:43:08 -0400 Received: from e4.ny.us.ibm.com ([32.97.182.144]:60980) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMli7-0005Bh-Lq for qemu-devel@nongnu.org; Wed, 18 May 2011 14:43:07 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e4.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p4IIMEMN024550 for ; Wed, 18 May 2011 14:22:14 -0400 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p4IIh4Ws109678 for ; Wed, 18 May 2011 14:43:04 -0400 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p4ICfgwp032699 for ; Wed, 18 May 2011 06:41:42 -0600 Received: from oc6675851006.ibm.com (dyn9047029068.beaverton.ibm.com [9.47.29.68]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id p4ICfgcj032638 for ; Wed, 18 May 2011 06:41:42 -0600 Message-ID: <4DD41301.5060804@linux.vnet.ibm.com> Date: Wed, 18 May 2011 11:42:09 -0700 From: Venkateswararao Jujjuri MIME-Version: 1.0 References: <1305661431-21289-1-git-send-email-jvrao@linux.vnet.ibm.com> <1305661431-21289-7-git-send-email-jvrao@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [V2 06/25] [virtio-9p] coroutines for readlink List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org On 05/18/2011 02:43 AM, Stefan Hajnoczi wrote: > On Tue, May 17, 2011 at 8:43 PM, Venkateswararao Jujjuri (JV) > wrote: >> Signed-off-by: Venkateswararao Jujjuri " >> --- >> Makefile.objs | 2 +- >> hw/9pfs/cofs.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> hw/9pfs/virtio-9p-coth.h | 1 + >> hw/9pfs/virtio-9p.c | 27 ++++----------------------- >> hw/9pfs/virtio-9p.h | 3 ++- >> 5 files changed, 50 insertions(+), 25 deletions(-) >> create mode 100644 hw/9pfs/cofs.c >> >> diff --git a/Makefile.objs b/Makefile.objs >> index 96f6a24..36005bb 100644 >> --- a/Makefile.objs >> +++ b/Makefile.objs >> @@ -297,7 +297,7 @@ hw-obj-$(CONFIG_SOUND) += $(sound-obj-y) >> 9pfs-nested-$(CONFIG_VIRTFS) = virtio-9p.o virtio-9p-debug.o >> 9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-local.o virtio-9p-xattr.o >> 9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-xattr-user.o virtio-9p-posix-acl.o >> -9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-coth.o >> +9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-coth.o cofs.o >> >> hw-obj-$(CONFIG_REALLY_VIRTFS) += $(addprefix 9pfs/, $(9pfs-nested-y)) >> $(addprefix 9pfs/, $(9pfs-nested-y)): QEMU_CFLAGS+=$(GLIB_CFLAGS) >> diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c >> new file mode 100644 >> index 0000000..6d94673 >> --- /dev/null >> +++ b/hw/9pfs/cofs.c >> @@ -0,0 +1,42 @@ >> + >> +/* >> + * Virtio 9p backend >> + * >> + * Copyright IBM, Corp. 2011 >> + * >> + * Authors: >> + * Aneesh Kumar K.V >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2. See >> + * the COPYING file in the top-level directory. >> + * >> + */ >> + >> +#include "fsdev/qemu-fsdev.h" >> +#include "qemu-thread.h" >> +#include "qemu-coroutine.h" >> +#include "virtio-9p-coth.h" >> + >> +int v9fs_co_readlink(V9fsState *s, V9fsString *path, V9fsString *buf) >> +{ >> + int err; >> + ssize_t len; >> + V9fsString tbuf; >> + >> + tbuf.data = qemu_malloc(PATH_MAX); > Why introduce tbuf when the buf is available? You end up having to > copy back fields at the end of the function and load from an > uninitialized address (tbuf.size) in the error case. tbuf is introduced for re-entrent purpose. We should be calling v9fs_string_init() on this though. >> @@ -299,7 +282,7 @@ static void v9fs_string_init(V9fsString *str) >> str->size = 0; >> } >> >> -static void v9fs_string_free(V9fsString *str) >> +void v9fs_string_free(V9fsString *str) >> { >> qemu_free(str->data); >> str->data = NULL; >> @@ -421,7 +404,7 @@ v9fs_string_sprintf(V9fsString *str, const char *fmt, ...) >> str->size = err; >> } >> >> -static void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs) >> +void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs) >> { >> v9fs_string_free(lhs); >> v9fs_string_sprintf(lhs, "%s", rhs->data); > Spurious changes? I don't see any users here. Yep. Good catch. >> @@ -1057,8 +1040,7 @@ static int stat_to_v9stat(V9fsState *s, V9fsString *name, >> v9fs_string_null(&v9stat->extension); >> >> if (v9stat->mode& P9_STAT_MODE_SYMLINK) { >> - ssize_t symlink_len; >> - err = v9fs_do_readlink(s, name,&v9stat->extension,&symlink_len); >> + err = v9fs_co_readlink(s, name,&v9stat->extension); >> if (err< 0) { >> return err; >> } >> @@ -3591,7 +3573,6 @@ static void v9fs_readlink(void *opaque) >> int32_t fid; >> int err = 0; >> V9fsFidState *fidp; >> - ssize_t symlink_len; >> >> pdu_unmarshal(pdu, offset, "d",&fid); >> fidp = lookup_fid(pdu->s, fid); >> @@ -3601,7 +3582,7 @@ static void v9fs_readlink(void *opaque) >> } >> >> v9fs_string_init(&target); >> - err = v9fs_do_readlink(pdu->s,&fidp->path,&target,&symlink_len); >> + err = v9fs_co_readlink(pdu->s,&fidp->path,&target); >> if (err< 0) { >> goto out; >> } > Ah, the unused length argument from patch 5 has been removed. It > would be nice to do fixes earlier rather than later in the patch > series, otherwise time is spent reviewing and commenting on code which > gets removed. > The previous patch is just to move errno. So we thought of doing here while we are introducing coroutines. But thinking now we could have done either place. Thanks, JV > Stefan >