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

[M. Mohan Kumar]

On Wednesday, October 12, 2011 07:38:25 PM Aneesh Kumar K.V wrote:
> 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 ?
It was part of alignment change for above line

> 
> > +#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 ?
Yes, we can use same variable.
> 
> >  } 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 ?

Thanks, I will include the check for 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