On Thu, 09 Jun 2011 17:27:00 -0700, Venkateswararao Jujjuri
<jv...@linux.vnet.ibm.com> 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<aneesh.ku...@linux.vnet.ibm.com>
> > ---
> > 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
