[Qemu-devel] [RFC/PATCH] virtio-9p: Add Read only support for 9p export.

[Qemu-devel] [RFC/PATCH] virtio-9p: Add Read only support for 9p export.

[M. Mohan Kumar]

A new fsdev parameter "access" is introduced to control accessing 9p export.
access=ro|rw can be used to specify the access type. By default rw access
is given to 9p export.

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 fsdev/file-op-9p.h         |    1 +
 fsdev/qemu-fsdev.c         |   17 +++++++++++++++-
 fsdev/qemu-fsdev.h         |    1 +
 hw/9pfs/virtio-9p-device.c |    5 ++++
 hw/9pfs/virtio-9p.c        |   46 ++++++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/virtio-9p.h        |    2 +
 qemu-config.c              |    7 ++++++
 vl.c                       |    1 +
 8 files changed, 79 insertions(+), 1 deletions(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index af9daf7..b1642eb 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -55,6 +55,7 @@ typedef struct FsContext
     SecModel fs_sm;
     uid_t uid;
     struct xattr_operations **xops;
+    int flags;
 } FsContext;
 
 void cred_init(FsCred *);
diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index 0b33290..259a03b 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -28,11 +28,12 @@ static FsTypeTable FsTypes[] = {
 int qemu_fsdev_add(QemuOpts *opts)
 {
     struct FsTypeListEntry *fsle;
-    int i;
+    int i, flags;
     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 *access = qemu_opt_get(opts, "access");
 
     if (!fsdev_id) {
         fprintf(stderr, "fsdev: No id specified\n");
@@ -65,12 +66,26 @@ int qemu_fsdev_add(QemuOpts *opts)
         return -1;
     }
 
+    flags = 0;
+    if (access) {
+        if (!strcmp(access, "ro")) {
+            flags |= FS_RDONLY;
+        } else if (!strcmp(access, "rw")) {
+            flags &= ~FS_RDONLY;
+        } else {
+            fprintf(stderr, "fsdev: Invalid access method specified.\n");
+            fprintf(stderr, "fsdev: access=ro|rw\n");
+            return -1;
+        }
+    }
+
     fsle = qemu_malloc(sizeof(*fsle));
 
     fsle->fse.fsdev_id = qemu_strdup(fsdev_id);
     fsle->fse.path = qemu_strdup(path);
     fsle->fse.security_model = qemu_strdup(sec_model);
     fsle->fse.ops = FsTypes[i].ops;
+    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 f9f08d3..1ca3385 100644
--- a/fsdev/qemu-fsdev.h
+++ b/fsdev/qemu-fsdev.h
@@ -42,6 +42,7 @@ typedef struct FsTypeEntry {
     char *path;
     char *security_model;
     FileOperations *ops;
+    int flags;
 } FsTypeEntry;
 
 typedef struct FsTypeListEntry {
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index f985486..409a9fe 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -107,6 +107,11 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
         s->ctx.xops = none_xattr_ops;
     }
 
+    s->ctx.flags = 0;
+    if (fse->flags & FS_RDONLY) {
+        s->ctx.flags |= FS_RDONLY;
+    }
+
     if (lstat(fse->path, &stat)) {
         fprintf(stderr, "share path %s does not exist\n", fse->path);
         exit(1);
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 21865b1..4a9790c 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -1192,6 +1192,14 @@ static void v9fs_fix_path(V9fsString *dst, V9fsString *src, int len)
     v9fs_string_free(&str);
 }
 
+static inline int is_ro_export(FsContext *fs_ctx)
+{
+        if (fs_ctx->flags & FS_RDONLY) {
+                return 1;
+        }
+        return 0;
+}
+
 static void v9fs_version(V9fsState *s, V9fsPDU *pdu)
 {
     V9fsString version;
@@ -1734,6 +1742,13 @@ static void v9fs_open_post_lstat(V9fsState *s, V9fsOpenState *vs, int err)
         vs->fidp->fs.dir = v9fs_do_opendir(s, &vs->fidp->path);
         v9fs_open_post_opendir(s, vs, err);
     } else {
+        if (is_ro_export(&s->ctx)) {
+            if (vs->mode & O_WRONLY || vs->mode & O_RDWR ||
+                            vs->mode & O_APPEND) {
+                err = -EROFS;
+                goto out;
+            }
+        }
         if (s->proto_version == V9FS_PROTO_2000L) {
             flags = vs->mode;
             flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
@@ -3606,6 +3621,33 @@ static pdu_handler_t *pdu_handlers[] = {
     [P9_TREMOVE] = v9fs_remove,
 };
 
+static inline int is_read_only_op(int id)
+{
+    switch (id) {
+        case P9_TREADDIR:
+        case P9_TSTATFS:
+        case P9_TGETATTR:
+        case P9_TXATTRWALK:
+        case P9_TLOCK:
+        case P9_TGETLOCK:
+        case P9_TREADLINK:
+        case P9_TVERSION:
+        case P9_TLOPEN:
+        case P9_TATTACH:
+        case P9_TSTAT:
+        case P9_TWALK:
+        case P9_TCLUNK:
+        case P9_TFSYNC:
+        case P9_TOPEN:
+        case P9_TREAD:
+        case P9_TAUTH:
+        case P9_TFLUSH:
+            return 1;
+        default:
+            return 0;
+    }
+}
+
 static void submit_pdu(V9fsState *s, V9fsPDU *pdu)
 {
     pdu_handler_t *handler;
@@ -3619,6 +3661,10 @@ static void submit_pdu(V9fsState *s, V9fsPDU *pdu)
     handler = pdu_handlers[pdu->id];
     BUG_ON(handler == NULL);
 
+    if (is_ro_export(&s->ctx) && !is_read_only_op(pdu->id)) {
+        complete_pdu(s, pdu, -EROFS);
+        return;
+    }
     handler(s, pdu);
 }
 
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index d94c32c..5b58037 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -511,4 +511,6 @@ static inline size_t do_pdu_unpack(void *dst, struct iovec *sg, int sg_count,
 
 extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq);
 
+#define FS_RDONLY   0x01
+
 #endif
diff --git a/qemu-config.c b/qemu-config.c
index 14d3419..52ed236 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -170,7 +170,11 @@ QemuOptsList qemu_fsdev_opts = {
         }, {
             .name = "security_model",
             .type = QEMU_OPT_STRING,
+        }, {
+            .name = "access",
+            .type = QEMU_OPT_STRING,
         },
+
         { /*End of list */ }
     },
 };
@@ -192,6 +196,9 @@ QemuOptsList qemu_virtfs_opts = {
         }, {
             .name = "security_model",
             .type = QEMU_OPT_STRING,
+        }, {
+            .name = "access",
+            .type = QEMU_OPT_STRING,
         },
 
         { /*End of list */ }
diff --git a/vl.c b/vl.c
index 6b9a2f6..9858bab 100644
--- a/vl.c
+++ b/vl.c
@@ -2483,6 +2483,7 @@ int main(int argc, char **argv, char **envp)
                 qemu_opt_set(fsdev, "security_model",
                              qemu_opt_get(opts, "security_model"));
 
+                qemu_opt_set(fsdev, "access", qemu_opt_get(opts, "access"));
                 device = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
                 qemu_opt_set(device, "driver", "virtio-9p-pci");
                 qemu_opt_set(device, "fsdev",
-- 
1.7.4

Re: [Qemu-devel] [RFC/PATCH] virtio-9p: Add Read only support for 9p export.

[Stefan Hajnoczi]

On Mon, May 23, 2011 at 8:58 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> A new fsdev parameter "access" is introduced to control accessing 9p export.
> access=ro|rw can be used to specify the access type. By default rw access
> is given to 9p export.

It would be consistent with -drive to use readonly=on|off (default:
off).  Do you have any future access modes in mind that require more
than readonly on|off?

It seems the client does not know ahead of time that the mount is
read-only.  I wonder if this exposes any weirdness compared to
actually mounting ro on the client side (i.e. operations that appear
to start okay but then fail with -EROFS at a different point than if
you had mounted ro)?  It could cause apps to error in new ways.

> +static inline int is_ro_export(FsContext *fs_ctx)
> +{
> +        if (fs_ctx->flags & FS_RDONLY) {
> +                return 1;
> +        }
> +        return 0;
> +}

Please use bool and fix indentation:

static inline bool is_ro_export(FsContext *fs_ctx)
{
    return fs_ctx->flags & FS_RDONLY;
}

> +
>  static void v9fs_version(V9fsState *s, V9fsPDU *pdu)
>  {
>     V9fsString version;
> @@ -1734,6 +1742,13 @@ static void v9fs_open_post_lstat(V9fsState *s, V9fsOpenState *vs, int err)
>         vs->fidp->fs.dir = v9fs_do_opendir(s, &vs->fidp->path);
>         v9fs_open_post_opendir(s, vs, err);
>     } else {
> +        if (is_ro_export(&s->ctx)) {
> +            if (vs->mode & O_WRONLY || vs->mode & O_RDWR ||
> +                            vs->mode & O_APPEND) {
> +                err = -EROFS;
> +                goto out;
> +            }
> +        }
>         if (s->proto_version == V9FS_PROTO_2000L) {
>             flags = vs->mode;
>             flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
> @@ -3606,6 +3621,33 @@ static pdu_handler_t *pdu_handlers[] = {
>     [P9_TREMOVE] = v9fs_remove,
>  };
>
> +static inline int is_read_only_op(int id)

bool

Stefan

Re: [Qemu-devel] [RFC/PATCH] virtio-9p: Add Read only support for 9p export.

[M. Mohan Kumar]

On Mon, May 23, 2011 at 11:08:34AM +0100, Stefan Hajnoczi wrote:
> On Mon, May 23, 2011 at 8:58 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> > A new fsdev parameter "access" is introduced to control accessing 9p export.
> > access=ro|rw can be used to specify the access type. By default rw access
> > is given to 9p export.
> 
> It would be consistent with -drive to use readonly=on|off (default:
> off).  Do you have any future access modes in mind that require more
> than readonly on|off?

You mean to use 'readonly=on|off' instead of 'access=ro|rw' ?

> 
> It seems the client does not know ahead of time that the mount is
> read-only.  I wonder if this exposes any weirdness compared to
> actually mounting ro on the client side (i.e. operations that appear
> to start okay but then fail with -EROFS at a different point than if
> you had mounted ro)?  It could cause apps to error in new ways.
> 

When a NFS share is exported as read-only, client will come to know about
that when it does some write operation. IMHO its not an issue.

> > +static inline int is_ro_export(FsContext *fs_ctx)
> > +{
> > +        if (fs_ctx->flags & FS_RDONLY) {
> > +                return 1;
> > +        }
> > +        return 0;
> > +}
> 
> Please use bool and fix indentation:
Ok
> 
> static inline bool is_ro_export(FsContext *fs_ctx)
> {
>     return fs_ctx->flags & FS_RDONLY;
> }
> 
> > +
> >  static void v9fs_version(V9fsState *s, V9fsPDU *pdu)
> >  {
> >     V9fsString version;
> > @@ -1734,6 +1742,13 @@ static void v9fs_open_post_lstat(V9fsState *s, V9fsOpenState *vs, int err)
> >         vs->fidp->fs.dir = v9fs_do_opendir(s, &vs->fidp->path);
> >         v9fs_open_post_opendir(s, vs, err);
> >     } else {
> > +        if (is_ro_export(&s->ctx)) {
> > +            if (vs->mode & O_WRONLY || vs->mode & O_RDWR ||
> > +                            vs->mode & O_APPEND) {
> > +                err = -EROFS;
> > +                goto out;
> > +            }
> > +        }
> >         if (s->proto_version == V9FS_PROTO_2000L) {
> >             flags = vs->mode;
> >             flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
> > @@ -3606,6 +3621,33 @@ static pdu_handler_t *pdu_handlers[] = {
> >     [P9_TREMOVE] = v9fs_remove,
> >  };
> >
> > +static inline int is_read_only_op(int id)
> 
> bool
Ok

Re: [Qemu-devel] [RFC/PATCH] virtio-9p: Add Read only support for 9p export.

[Stefan Hajnoczi]

On Mon, May 23, 2011 at 12:49 PM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> On Mon, May 23, 2011 at 11:08:34AM +0100, Stefan Hajnoczi wrote:
>> On Mon, May 23, 2011 at 8:58 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
>> > A new fsdev parameter "access" is introduced to control accessing 9p export.
>> > access=ro|rw can be used to specify the access type. By default rw access
>> > is given to 9p export.
>>
>> It would be consistent with -drive to use readonly=on|off (default:
>> off).  Do you have any future access modes in mind that require more
>> than readonly on|off?
>
> You mean to use 'readonly=on|off' instead of 'access=ro|rw' ?

Yes.

>
>>
>> It seems the client does not know ahead of time that the mount is
>> read-only.  I wonder if this exposes any weirdness compared to
>> actually mounting ro on the client side (i.e. operations that appear
>> to start okay but then fail with -EROFS at a different point than if
>> you had mounted ro)?  It could cause apps to error in new ways.
>>
>
> When a NFS share is exported as read-only, client will come to know about
> that when it does some write operation. IMHO its not an issue.

If it works the same way on NFS then great.

Stefan