Re: [Qemu-devel] [PATCH V4 2/2] hw/9pfs: Add readonly support for 9p export

[Aneesh Kumar K.V]

On Wed, 12 Oct 2011 13:23:34 +0530, "M. Mohan Kumar" <address@hidden> wrote:
> A new fsdev parameter "readonly" is introduced to control accessing 9p export.
> readonly=on|off can be used to specify the access type. By default rw
> access is given.
> 
> Signed-off-by: M. Mohan Kumar <address@hidden>
> ---
> Changes from previous version V3:
> * Use opt_set_bool function to set readonly option
> * Change the flag from MS_READONLY to 9p specific
> 
> Change from previous version V2:
> * QEMU_OPT_BOOL is used for readdonly parameter
> 
> Changes from previous version:
> * Use "readonly" option instead of "access"
> * Change function return type to boolean where its needed
> 
>  fsdev/file-op-9p.h         |    3 +-
>  fsdev/qemu-fsdev.c         |   12 +++++++++-
>  fsdev/qemu-fsdev.h         |    1 +
>  hw/9pfs/virtio-9p-device.c |    3 ++
>  hw/9pfs/virtio-9p.c        |   46 
> ++++++++++++++++++++++++++++++++++++++++++++
>  qemu-config.c              |    7 ++++++
>  vl.c                       |    2 +
>  7 files changed, 71 insertions(+), 3 deletions(-)
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 33fb07f..b75290d 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -58,7 +58,8 @@ typedef struct extended_ops {
>  } extended_ops;
> 
>  /* FsContext flag values */
> -#define PATHNAME_FSCONTEXT 0x1
> +#define PATHNAME_FSCONTEXT  0x1

why ?

> +#define P9_RDONLY_EXPORT    0x2
> 
>  /* cache flags */
>  #define V9FS_WRITETHROUGH_CACHE 0x1
> diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
> index d08ba9c..f8a8227 100644
> --- a/fsdev/qemu-fsdev.c
> +++ b/fsdev/qemu-fsdev.c
> @@ -29,13 +29,13 @@ static FsTypeTable FsTypes[] = {
>  int qemu_fsdev_add(QemuOpts *opts)
>  {
>      struct FsTypeListEntry *fsle;
> -    int i;
> +    int i, flags = 0;
>      const char *fsdev_id = qemu_opts_id(opts);
>      const char *fstype = qemu_opt_get(opts, "fstype");
>      const char *path = qemu_opt_get(opts, "path");
>      const char *sec_model = qemu_opt_get(opts, "security_model");
>      const char *cache = qemu_opt_get(opts, "cache");
> -
> +    int rdonly = qemu_opt_get_bool(opts, "readonly", 0);
> 
>      if (!fsdev_id) {
>          fprintf(stderr, "fsdev: No id specified\n");
> @@ -76,6 +76,14 @@ int qemu_fsdev_add(QemuOpts *opts)
>              fsle->fse.cache_flags = V9FS_WRITETHROUGH_CACHE;
>          }
>      }
> +    if (rdonly) {
> +        flags |= P9_RDONLY_EXPORT;
> +    } else {
> +        flags &= ~P9_RDONLY_EXPORT;
> +    }
> +
> +    fsle->fse.flags = flags;
> +
>      QTAILQ_INSERT_TAIL(&fstype_entries, fsle, next);
>      return 0;
>  }
> diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h
> index 0f67880..2938eaf 100644
> --- a/fsdev/qemu-fsdev.h
> +++ b/fsdev/qemu-fsdev.h
> @@ -44,6 +44,7 @@ typedef struct FsTypeEntry {
>      char *security_model;
>      int cache_flags;
>      FileOperations *ops;
> +    int flags;

do we need extra flags ? Why not use cache_flags ? renaming that to
export_flags ?

>  } FsTypeEntry;
> 
>  typedef struct FsTypeListEntry {
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 1846e36..336292c 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -125,6 +125,9 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf 
> *conf)
>      s->tag_len = len;
>      s->ctx.uid = -1;
>      s->ctx.flags = 0;
> +    if (fse->flags & P9_RDONLY_EXPORT) {
> +        s->ctx.flags |= P9_RDONLY_EXPORT;
> +    }
> 
>      s->ops = fse->ops;
>      s->vdev.get_features = virtio_9p_get_features;
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 47ed2f1..9f15787 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -1271,6 +1271,11 @@ static void v9fs_fix_path(V9fsPath *dst, V9fsPath 
> *src, int len)
>      dst->size++;
>  }
> 
> +static inline bool is_ro_export(FsContext *fs_ctx)
> +{
> +    return fs_ctx->flags & P9_RDONLY_EXPORT;
> +}
> +
>  static void v9fs_version(void *opaque)
>  {
>      V9fsPDU *pdu = opaque;
> @@ -1690,6 +1695,14 @@ static void v9fs_open(void *opaque)
>          } else {
>              flags = omode_to_uflags(mode);
>          }
> +        if (is_ro_export(&s->ctx)) {
> +            if (mode & O_WRONLY || mode & O_RDWR || mode & O_APPEND) {
> +                err = -EROFS;
> +                goto out;
> +            } else {
> +                flags |= O_NOATIME;
> +            }
> +        }

What about O_TRUNC ?

>          err = v9fs_co_open(pdu, fidp, flags);
>          if (err < 0) {
>              goto out;
> @@ -3301,6 +3314,33 @@ static void v9fs_op_not_supp(void *opaque)
>      complete_pdu(pdu->s, pdu, -EOPNOTSUPP);
>  }
> 

-aneesh