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

["Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com>]

On Tue, 15 Mar 2011 10:38:31 +0000, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Mar 15, 2011 at 9:20 AM, Aneesh Kumar K. V
> <aneesh.kumar@linux.vnet.ibm.com> wrote:
> > On Sun, 13 Mar 2011 20:53:41 +0000, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> On Sun, Mar 13, 2011 at 7:06 PM, Aneesh Kumar K. V
> >> <aneesh.kumar@linux.vnet.ibm.com> wrote:
> >> > On Sun, 13 Mar 2011 15:46:29 +0000, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> >> On Sat, Mar 5, 2011 at 5:52 PM, Aneesh Kumar K.V
> >> >> <aneesh.kumar@linux.vnet.ibm.com> wrote:
> >> >> > @@ -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.
> >>
> >> Where does sharing happen, I didn't notice any code that shares fds
> >> between fids?
> >
> > That patch is not yet there. We can only share fd if they open flags
> > match. Hence making sure we open files on host with limited set of open
> > flags which enables us much better sharing.
> 
> Tracking open flags is fine and is needed for fd reclaim.  But
> splitting V9fsFidState into the V9fsFidMap structure and all the churn
> that causes to the code isn't necessary yet.  Please wait with that
> until you submit patches fd sharing.  The reason I make this
> suggestion is that everyone reading or working on the code until fd
> sharing is added now needs to deal with the V9fsFidMap structure which
> (currently) serves no purpose.

taken. will remove the patch from the series.

-aneesh