Re: [Qemu-devel] [PATCH 2/6] hw/9pfs: Don't free fid in clunk

[Aneesh Kumar K.V]

On Thu, 09 Jun 2011 17:27:00 -0700, Venkateswararao Jujjuri <address@hidden> 
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<address@hidden>
> > ---
> >   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