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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Sat, Mar 5, 2011 at 5:52 PM, Aneesh Kumar K.V
<aneesh.kumar@linux.vnet.ibm.com> wrote:
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
>  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?

Stefan