Re: [Qemu-devel] [PATCH -V3 1/8] hw/9pfs: Add V9fsfidmap in preparation for adding fd reclaim

[Aneesh Kumar K. V]

On Sun, 13 Mar 2011 15:46:29 +0000, Stefan Hajnoczi <address@hidden> wrote:
> On Sat, Mar 5, 2011 at 5:52 PM, Aneesh Kumar K.V
> <address@hidden> wrote:
> > Signed-off-by: Aneesh Kumar K.V <address@hidden>
> > ---
> >  hw/9pfs/virtio-9p.c |  327 
> > ++++++++++++++++++++++++++-------------------------
> >  hw/9pfs/virtio-9p.h |   22 +++-
> >  2 files changed, 184 insertions(+), 165 deletions(-)
> 
> I don't understand why this patch is necessary.  It introduces an
> intermediate structure that doesn't seem to be useful by itself.
> Don't we always need the V9fsFidState?
> 
> > @@ -185,17 +188,22 @@ typedef struct V9fsXattr
> >     int flags;
> >  } V9fsXattr;
> >
> > +typedef struct V9fsfidmap {
> 
> V9fsFidMap (naming convention)
> 
> > +    union {
> > +        int fd;
> > +        DIR *dir;
> > +        V9fsXattr xattr;
> > +    } fs;
> 
> The name "fs" is not meaningful.
> 
> > +    int fid_type;
> > +    V9fsString path;
> > +    int flags;
> > +} V9fsFidMap;
> > +
> >  struct V9fsFidState
> >  {
> > -    int fid_type;
> >     int32_t fid;
> > -    V9fsString path;
> > -    union {
> > -       int fd;
> > -       DIR *dir;
> > -       V9fsXattr xattr;
> > -    } fs;
> >     uid_t uid;
> > +    V9fsFidMap fsmap;
> 
> This name is confusing.  A "map" is usually a container that stores
> key/value pairs.  V9fsFidMapEntry would be clearer.  But then I
> thought that is what V9fsFidState is?

I am bad at naming. I wanted to indicate something that can be shared
across multiple fids and also indicate the local file system
"mapping"/data. I will take any suggestion.

-aneesh