From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42956) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QUpZ4-0008T8-8I for qemu-devel@nongnu.org; Thu, 09 Jun 2011 20:27:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QUpZ1-0001or-Py for qemu-devel@nongnu.org; Thu, 09 Jun 2011 20:27:06 -0400 Received: from e38.co.us.ibm.com ([32.97.110.159]:51492) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QUpZ1-0001oZ-6n for qemu-devel@nongnu.org; Thu, 09 Jun 2011 20:27:03 -0400 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by e38.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p5A0Ii14010675 for ; Thu, 9 Jun 2011 18:18:44 -0600 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p5A0R1ds161468 for ; Thu, 9 Jun 2011 18:27:01 -0600 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 p59IQX1F023980 for ; Thu, 9 Jun 2011 12:26:33 -0600 Message-ID: <4DF164D4.5070902@linux.vnet.ibm.com> Date: Thu, 09 Jun 2011 17:27:00 -0700 From: Venkateswararao Jujjuri MIME-Version: 1.0 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> In-Reply-To: <1307380618-1963-2-git-send-email-aneesh.kumar@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: "Aneesh Kumar K.V" Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org 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? > 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() > +{ > V9fsFidState **fidpp, *fidp; > > for (fidpp =&s->fid_list; *fidpp; fidpp =&(*fidpp)->next) { > @@ -314,20 +337,10 @@ static int free_fid(V9fsState *s, int32_t fid) > if (*fidpp == NULL) { > return -ENOENT; > } > - > fidp = *fidpp; > *fidpp = fidp->next; > - > - 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; > + fidp->clunked = 1; > + return 0; > } > > #define P9_QID_TYPE_DIR 0x80 > @@ -1022,14 +1035,13 @@ static void v9fs_attach(void *opaque) > err = fid_to_qid(s, fidp,&qid); > if (err< 0) { > err = -EINVAL; > - put_fid(fidp); > free_fid(s, fid); > - goto out_nofid; > + goto out; > } > offset += pdu_marshal(pdu, offset, "Q",&qid); > err = offset; > - put_fid(fidp); > - > +out: > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, err); > v9fs_string_free(&uname); > @@ -1066,7 +1078,7 @@ static void v9fs_stat(void *opaque) > err = offset; > v9fs_stat_free(&v9stat); > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, err); > } > @@ -1102,7 +1114,7 @@ static void v9fs_getattr(void *opaque) > retval = offset; > retval += pdu_marshal(pdu, offset, "A",&v9stat_dotl); > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, retval); > } > @@ -1196,7 +1208,7 @@ static void v9fs_setattr(void *opaque) > } > err = offset; > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, err); > } > @@ -1279,9 +1291,7 @@ static void v9fs_walk(void *opaque) > v9fs_string_copy(&newfidp->path,&path); > err = v9fs_co_lstat(s,&newfidp->path,&stbuf); > if (err< 0) { > - put_fid(newfidp); > free_fid(s, newfidp->fid); > - newfidp = NULL; > v9fs_string_free(&path); > goto out; > } > @@ -1291,9 +1301,9 @@ static void v9fs_walk(void *opaque) > } > err = v9fs_walk_marshal(pdu, nwnames, qids); > out: > - put_fid(fidp); > + put_fid(s, fidp); > if (newfidp) { > - put_fid(newfidp); > + put_fid(s, newfidp); > } > out_nofid: > complete_pdu(s, pdu, err); > @@ -1383,7 +1393,7 @@ static void v9fs_open(void *opaque) > err = offset; > } > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, err); > } > @@ -1437,7 +1447,7 @@ static void v9fs_lcreate(void *opaque) > offset += pdu_marshal(pdu, offset, "Qd",&qid, iounit); > err = offset; > out: > - put_fid(fidp); > + put_fid(pdu->s, fidp); > out_nofid: > complete_pdu(pdu->s, pdu, err); > v9fs_string_free(&name); > @@ -1465,7 +1475,7 @@ static void v9fs_fsync(void *opaque) > err = offset; > } > out: > - put_fid(fidp); > + put_fid(s, fidp); > complete_pdu(s, pdu, err); > } > > @@ -1474,16 +1484,25 @@ static void v9fs_clunk(void *opaque) > int err; > int32_t fid; > size_t offset = 7; > + V9fsFidState *fidp; > V9fsPDU *pdu = opaque; > V9fsState *s = pdu->s; > > pdu_unmarshal(pdu, offset, "d",&fid); > - err = free_fid(s, fid); > + > + fidp = get_fid(s, fid); > + if (fidp == NULL) { > + err = -ENOENT; > + goto out_nofid; > + } > + err = free_fid(s, fidp->fid); > if (err< 0) { > goto out; > } > err = offset; > out: > + put_fid(s, fidp); > +out_nofid: > complete_pdu(s, pdu, err); > } > > @@ -1638,7 +1657,7 @@ static void v9fs_read(void *opaque) > err = -EINVAL; > } > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, err); > } > @@ -1747,7 +1766,7 @@ static void v9fs_readdir(void *opaque) > retval += pdu_marshal(pdu, offset, "d", count); > retval += count; > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, retval); > } > @@ -1857,7 +1876,7 @@ static void v9fs_write(void *opaque) > offset += pdu_marshal(pdu, offset, "d", total); > err = offset; > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, err); > } > @@ -1923,10 +1942,10 @@ static void v9fs_create(void *opaque) > } > err = v9fs_co_link(pdu->s,&nfidp->path,&fullname); > if (err< 0) { > - put_fid(nfidp); > + put_fid(pdu->s, nfidp); > goto out; > } > - put_fid(nfidp); > + put_fid(pdu->s, nfidp); > } else if (perm& P9_STAT_MODE_DEVICE) { > char ctype; > uint32_t major, minor; > @@ -1990,7 +2009,7 @@ static void v9fs_create(void *opaque) > err = offset; > > out: > - put_fid(fidp); > + put_fid(pdu->s, fidp); > out_nofid: > complete_pdu(pdu->s, pdu, err); > v9fs_string_free(&name); > @@ -2035,7 +2054,7 @@ static void v9fs_symlink(void *opaque) > offset += pdu_marshal(pdu, offset, "Q",&qid); > err = offset; > out: > - put_fid(dfidp); > + put_fid(pdu->s, dfidp); > out_nofid: > complete_pdu(pdu->s, pdu, err); > v9fs_string_free(&name); > @@ -2086,7 +2105,7 @@ static void v9fs_link(void *opaque) > v9fs_string_free(&fullname); > > out: > - put_fid(dfidp); > + put_fid(s, dfidp); > out_nofid: > v9fs_string_free(&name); > complete_pdu(s, pdu, err); > @@ -2113,8 +2132,8 @@ static void v9fs_remove(void *opaque) > } > > /* For TREMOVE we need to clunk the fid even on failed remove */ > - put_fid(fidp); > free_fid(pdu->s, fidp->fid); > + put_fid(pdu->s, fidp); > out_nofid: > complete_pdu(pdu->s, pdu, err); > } > @@ -2185,7 +2204,7 @@ static int v9fs_complete_rename(V9fsState *s, V9fsFidState *fidp, > } > out: > if (dirfidp) { > - put_fid(dirfidp); > + put_fid(s, dirfidp); > } > out_nofid: > return err; > @@ -2216,7 +2235,7 @@ static void v9fs_rename(void *opaque) > err = offset; > } > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, err); > v9fs_string_free(&name); > @@ -2305,7 +2324,7 @@ static void v9fs_wstat(void *opaque) > } > err = offset; > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > v9fs_stat_free(&v9stat); > complete_pdu(s, pdu, err); > @@ -2379,7 +2398,7 @@ static void v9fs_statfs(void *opaque) > retval = offset; > retval += v9fs_fill_statfs(s, pdu,&stbuf); > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, retval); > return; > @@ -2425,7 +2444,7 @@ static void v9fs_mknod(void *opaque) > err = offset; > err += pdu_marshal(pdu, offset, "Q",&qid); > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, err); > v9fs_string_free(&fullname); > @@ -2473,7 +2492,7 @@ static void v9fs_lock(void *opaque) > } > status = P9_LOCK_SUCCESS; > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > err = offset; > err += pdu_marshal(pdu, offset, "b", status); > @@ -2515,7 +2534,7 @@ static void v9fs_getlock(void *opaque) > &glock->client_id); > err = offset; > out: > - put_fid(fidp); > + put_fid(s, fidp); > out_nofid: > complete_pdu(s, pdu, err); > qemu_free(glock); > @@ -2555,7 +2574,7 @@ static void v9fs_mkdir(void *opaque) > offset += pdu_marshal(pdu, offset, "Q",&qid); > err = offset; > out: > - put_fid(fidp); > + put_fid(pdu->s, fidp); > out_nofid: > complete_pdu(pdu->s, pdu, err); > v9fs_string_free(&fullname); > @@ -2593,9 +2612,7 @@ static void v9fs_xattrwalk(void *opaque) > size = v9fs_co_llistxattr(s,&xattr_fidp->path, NULL, 0); > if (size< 0) { > err = size; > - put_fid(xattr_fidp); > free_fid(s, xattr_fidp->fid); > - xattr_fidp = NULL; > goto out; > } > /* > @@ -2610,9 +2627,7 @@ static void v9fs_xattrwalk(void *opaque) > xattr_fidp->fs.xattr.value, > xattr_fidp->fs.xattr.len); > if (err< 0) { > - put_fid(xattr_fidp); > free_fid(s, xattr_fidp->fid); > - xattr_fidp = NULL; > goto out; > } > } > @@ -2627,9 +2642,7 @@ static void v9fs_xattrwalk(void *opaque) > &name, NULL, 0); > if (size< 0) { > err = size; > - put_fid(xattr_fidp); > free_fid(s, xattr_fidp->fid); > - xattr_fidp = NULL; > goto out; > } > /* > @@ -2644,9 +2657,7 @@ static void v9fs_xattrwalk(void *opaque) > &name, xattr_fidp->fs.xattr.value, > xattr_fidp->fs.xattr.len); > if (err< 0) { > - put_fid(xattr_fidp); > free_fid(s, xattr_fidp->fid); > - xattr_fidp = NULL; > goto out; > } > } > @@ -2654,9 +2665,9 @@ static void v9fs_xattrwalk(void *opaque) > err = offset; > } > out: > - put_fid(file_fidp); > + put_fid(s, file_fidp); > if (xattr_fidp) { > - put_fid(xattr_fidp); > + put_fid(s, xattr_fidp); > } > out_nofid: > complete_pdu(s, pdu, err); > @@ -2698,7 +2709,7 @@ static void v9fs_xattrcreate(void *opaque) > xattr_fidp->fs.xattr.value = NULL; > } > err = offset; > - put_fid(file_fidp); > + put_fid(s, file_fidp); > out_nofid: > complete_pdu(s, pdu, err); > v9fs_string_free(&name); > @@ -2729,7 +2740,7 @@ static void v9fs_readlink(void *opaque) > err = offset; > v9fs_string_free(&target); > out: > - put_fid(fidp); > + put_fid(pdu->s, fidp); > out_nofid: > complete_pdu(pdu->s, pdu, err); > } > diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h > index ce93c20..e16e5f4 100644 > --- a/hw/9pfs/virtio-9p.h > +++ b/hw/9pfs/virtio-9p.h > @@ -204,6 +204,7 @@ struct V9fsFidState > } fs; > uid_t uid; > int ref; > + int clunked; > V9fsFidState *next; > }; > > @@ -362,11 +363,5 @@ static inline size_t do_pdu_unpack(void *dst, struct iovec *sg, int sg_count, > return pdu_packunpack(dst, sg, sg_count, offset, size, 0); > } > > -static inline void put_fid(V9fsFidState *fidp) > -{ > - BUG_ON(!fidp->ref); > - fidp->ref--; > -} > - > extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq); > #endif