Re: [Qemu-devel] [RFC PATCH -V1 4/4] hw/9pfs: Add open flag to fid

[Venkateswararao Jujjuri (JV)]

On 2/5/2011 10:08 AM, Aneesh Kumar K.V wrote:
> We use this flag when we reopen the file. We need
> to track open flag because if the open request have
> flags like O_SYNC, we want to open the file with same flag
> in host too
> 
> Signed-off-by: Aneesh Kumar K.V <address@hidden>
> ---
>  hw/9pfs/virtio-9p.c |   57 
> ++++++++++++++++++++++++++++++++++++++++++---------
>  hw/9pfs/virtio-9p.h |    1 +
>  2 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 1518e00..cbdf705 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -22,6 +22,8 @@
>  int debug_9p_pdu;
>  static void v9fs_reclaim_fd(V9fsState *s);
> 
> +#define PASS_OPEN_FLAG (O_SYNC | O_DSYNC | O_RSYNC |  \
> +                        O_EXCL)
>  enum {
>      Oread   = 0x00,
>      Owrite  = 0x01,
> @@ -68,6 +70,24 @@ static int omode_to_uflags(int8_t mode)
>      return ret;
>  }
> 
> +static int get_dotl_openflags(int oflags)
> +{
> +    int flags;
> +    /*
> +     * Since we can share the fd between multiple fids,
> +     * open the file in read write mode
> +     */
> +    flags = O_RDWR;
> +    /*
> +     * If the client asked for any of the below flags we
> +     * should open the file with same open flag
> +     */
> +    if (oflags & PASS_OPEN_FLAG) {
> +        flags =  oflags & PASS_OPEN_FLAG;
Isn't it

 flags |= oflags & PASS_OPEN_FLAG; ?
> +    }
> +    return flags;
> +}
> +
>  void cred_init(FsCred *credp)
>  {
>      credp->fc_uid = -1;
> @@ -452,9 +472,9 @@ static V9fsFidState *lookup_fid(V9fsState *s, int32_t fid)
>               * descriptors.
>               */
>              if (f->fsmap.fid_type == P9_FID_FILE) {
> -                /* FIXME!! should we remember the open flags ?*/
>                  if (f->fsmap.fs.fd == -1) {
> -                    f->fsmap.fs.fd = v9fs_do_open(s, &f->fsmap.path, O_RDWR);
> +                    f->fsmap.fs.fd = v9fs_do_open(s, &f->fsmap.path,
> +                                                  f->fsmap.open_flags);
>                  }
>              }
>              /*
> @@ -1811,14 +1831,19 @@ static void v9fs_open_post_lstat(V9fsState *s, 
> V9fsOpenState *vs, int err)
>          v9fs_open_post_opendir(s, vs, err);
>      } else {
>          if (s->proto_version == V9FS_PROTO_2000L) {
> -            flags = vs->mode;
> -            flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
> -            /* Ignore direct disk access hint until the server supports it. 
> */
> -            flags &= ~O_DIRECT;

This is more drastic change. So far we are ignoring only few flags..but with
this we are allowing only few flags. Need to understand the impact of this on
dotl protocol.

Also I would put this patch ahead of introducing fid reclaim patch.

> +            flags = get_dotl_openflags(vs->mode);
>          } else {
>              flags = omode_to_uflags(vs->mode);
>          }
>          vs->fidp->fsmap.fs.fd = v9fs_do_open(s, &vs->fidp->fsmap.path, 
> flags);
> +        vs->fidp->fsmap.open_flags = flags;
> +        if (flags & O_EXCL) {
> +            /*
> +             * We let the host file system do O_EXCL check
> +             * We should not reclaim such fd
> +             */
> +            vs->fidp->fsmap.flags  |= FID_NON_RECLAIMABLE;
> +        }
>          v9fs_open_post_open(s, vs, err);
>      }
>      return;
> @@ -1937,11 +1962,22 @@ static void v9fs_lcreate(V9fsState *s, V9fsPDU *pdu)
>      v9fs_string_sprintf(&vs->fullname, "%s/%s", vs->fidp->fsmap.path.data,
>               vs->name.data);
> 
> -    /* Ignore direct disk access hint until the server supports it. */
> -    flags &= ~O_DIRECT;
> -
> +    flags = get_dotl_openflags(flags);
>      vs->fidp->fsmap.fs.fd = v9fs_do_open2(s, vs->fullname.data, 
> vs->fidp->uid,
> -            gid, flags, mode);
> +                                          gid, flags, mode);
> +    /*
> +     * We don't want to recreate the in reclaim path. So remove
> +     * create flag
> +     */
> +    flags &= ~O_CREAT;
> +    vs->fidp->fsmap.open_flags = flags;
> +    if (flags & O_EXCL) {
> +        /*
> +         * We let the host file system do O_EXCL check
> +         * We should not reclaim such fd
> +         */
> +        vs->fidp->fsmap.flags  |= FID_NON_RECLAIMABLE;
> +    }
You can move above two steps to here by...
vs->fidp->fsmap.open_flags = flags & ~O_CREAT
>      v9fs_lcreate_post_do_open2(s, vs, err);
>      return;
> 
> @@ -2653,6 +2689,7 @@ static void v9fs_create_post_lstat(V9fsState *s, 
> V9fsCreateState *vs, int err)
>                                                -1,
>                                                
> omode_to_uflags(vs->mode)|O_CREAT,
>                                                vs->perm);
> +        vs->fidp->fsmap.open_flags = omode_to_uflags(vs->mode);

why are we not doing ~O_CREAT here?

- JV


>          v9fs_create_post_open2(s, vs, err);
>      }
> 
> diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
> index b2cd24b..c3cb75e 100644
> --- a/hw/9pfs/virtio-9p.h
> +++ b/hw/9pfs/virtio-9p.h
> @@ -196,6 +196,7 @@ typedef struct V9fsfidmap {
>          DIR *dir;
>          V9fsXattr xattr;
>      } fs;
> +    int open_flags;
>      int fid_type;
>      V9fsString path;
>      int flags;