From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53832) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QUv41-0007nR-9W for qemu-devel@nongnu.org; Fri, 10 Jun 2011 02:19:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QUv40-00057x-0g for qemu-devel@nongnu.org; Fri, 10 Jun 2011 02:19:25 -0400 Received: from e2.ny.us.ibm.com ([32.97.182.142]:48568) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QUv3z-000577-SD for qemu-devel@nongnu.org; Fri, 10 Jun 2011 02:19:23 -0400 Received: from d01relay03.pok.ibm.com (d01relay03.pok.ibm.com [9.56.227.235]) by e2.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p5A5wxAU014907 for ; Fri, 10 Jun 2011 01:58:59 -0400 Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay03.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p5A6JJBO120068 for ; Fri, 10 Jun 2011 02:19:19 -0400 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p5A6JJ1t019562 for ; Fri, 10 Jun 2011 02:19:19 -0400 From: "Aneesh Kumar K.V" In-Reply-To: <4DF164D4.5070902@linux.vnet.ibm.com> References: <1307380618-1963-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <1307380618-1963-2-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <4DF164D4.5070902@linux.vnet.ibm.com> Date: Fri, 10 Jun 2011 11:49:11 +0530 Message-ID: <87oc26mco0.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH 2/6] hw/9pfs: Don't free fid in clunk List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Venkateswararao Jujjuri Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org On Thu, 09 Jun 2011 17:27:00 -0700, Venkateswararao Jujjuri wrote: > On 06/06/2011 10:16 AM, Aneesh Kumar K.V wrote: > > On interrupted syscall on client we can end up having fid > > being acted upon by glib pool but still get a clunk request on that > > Couple of comments below. > > - JV > > > Signed-off-by: Aneesh Kumar K.V > > --- > > hw/9pfs/virtio-9p.c | 139 +++++++++++++++++++++++++++----------------------- > > hw/9pfs/virtio-9p.h | 7 +-- > > 2 files changed, 76 insertions(+), 70 deletions(-) > > > > diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c > > index 03d8664..f3127a5 100644 > > --- a/hw/9pfs/virtio-9p.c > > +++ b/hw/9pfs/virtio-9p.c > > @@ -237,12 +237,11 @@ static V9fsFidState *get_fid(V9fsState *s, int32_t fid) > > V9fsFidState *f; > > > > for (f = s->fid_list; f; f = f->next) { > > - if (f->fid == fid) { > > + if (f->fid == fid&& !f->clunked) { > > f->ref++; > > return f; > > } > > } > > - > > return NULL; > > } > > > > @@ -250,12 +249,12 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid) > > { > > V9fsFidState *f; > > > > - f = get_fid(s, fid); > > - if (f) { > > - f->ref--; > > - return NULL; > > + for (f = s->fid_list; f; f = f->next) { > > + /* If fid is already there return NULL */ > > + if (f->fid == fid&& !f->clunked) { > > + return NULL; > > + } > > } > > - > > f = qemu_mallocz(sizeof(V9fsFidState)); > Memory leak if we find a cluncked fid here? > More than that how do you handle this? clunk request happening parallel to alloc request ? is that possible. Only when the alloc succeed the client will find the fid initialized and only on initialized fid we send clunk request. > > f->fid = fid; > > f->fid_type = P9_FID_NONE; > > @@ -300,9 +299,33 @@ free_value: > > return retval; > > } > > > > -static int free_fid(V9fsState *s, int32_t fid) > > +static int release_fid(V9fsState *s, V9fsFidState *fidp) > > { > > int retval = 0; > > + > > + if (fidp->fid_type == P9_FID_FILE) { > > + retval = v9fs_co_close(s, fidp); > > + } else if (fidp->fid_type == P9_FID_DIR) { > > + retval = v9fs_co_closedir(s, fidp); > > + } else if (fidp->fid_type == P9_FID_XATTR) { > > + retval = v9fs_xattr_fid_clunk(s, fidp); > > + } > > + v9fs_string_free(&fidp->path); > > + qemu_free(fidp); > > + return retval; > > +} > > + > > +static void put_fid(V9fsState *s, V9fsFidState *fidp) > > +{ > > + BUG_ON(!fidp->ref); > > + fidp->ref--; > > + if (!fidp->ref&& fidp->clunked) { > > + release_fid(s, fidp); > > + } > > +} > > + > > +static int free_fid(V9fsState *s, int32_t fid) > With the changed semantics; I would suggest you to swap the names of > release_fid() and free_fid() > > Or even better > free_fid() -> clunk_fid() > release_fid() -> free_fid() ok -aneesh