Re: [Qemu-devel] [PATCH] hw/9pfs: Handle Security model parsing

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

On Wed, 12 Oct 2011 13:24:16 +0530, "M. Mohan Kumar" <mohan@in.ibm.com> wrote:
> Security model is needed only for 'local' fs driver.

Can you also cleanup that fstype -> fsdriver rename ? fsdriver seems
more appropriate.

> 
> Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
> ---
>  fsdev/qemu-fsdev.c         |    6 +----
>  fsdev/qemu-fsdev.h         |    1 +
>  hw/9pfs/virtio-9p-device.c |   47 ++++++++++++++++++++++---------------------
>  vl.c                       |   20 +++++++++++++++--
>  4 files changed, 43 insertions(+), 31 deletions(-)
> 
> diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
> index 36db127..d08ba9c 100644
> --- a/fsdev/qemu-fsdev.c
> +++ b/fsdev/qemu-fsdev.c
> @@ -58,11 +58,6 @@ int qemu_fsdev_add(QemuOpts *opts)
>          return -1;
>      }
> 
> -    if (!sec_model) {
> -        fprintf(stderr, "fsdev: No security_model specified.\n");
> -        return -1;
> -    }
> -
>      if (!path) {
>          fprintf(stderr, "fsdev: No path specified.\n");
>          return -1;
> @@ -72,6 +67,7 @@ int qemu_fsdev_add(QemuOpts *opts)
> 
>      fsle->fse.fsdev_id = g_strdup(fsdev_id);
>      fsle->fse.path = g_strdup(path);
> +    fsle->fse.fsdriver = g_strdup(fstype);

Why use it as a string ? Why can't this again be an export_flag. That
would help us to avoid that strdup 


>      fsle->fse.security_model = g_strdup(sec_model);
>      fsle->fse.ops = FsTypes[i].ops;
>      fsle->fse.cache_flags = 0;
> diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h
> index 9c440f2..0f67880 100644
> --- a/fsdev/qemu-fsdev.h
> +++ b/fsdev/qemu-fsdev.h
> @@ -40,6 +40,7 @@ typedef struct FsTypeTable {
>  typedef struct FsTypeEntry {
>      char *fsdev_id;
>      char *path;
> +    char *fsdriver;
>      char *security_model;
>      int cache_flags;
>      FileOperations *ops;
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index aac58ad..1846e36 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -83,29 +83,30 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
>          exit(1);
>      }
> 
> -    if (!strcmp(fse->security_model, "passthrough")) {
> -        /* Files on the Fileserver set to client user credentials */
> -        s->ctx.fs_sm = SM_PASSTHROUGH;
> -        s->ctx.xops = passthrough_xattr_ops;
> -    } else if (!strcmp(fse->security_model, "mapped")) {
> -        /* Files on the fileserver are set to QEMU credentials.
> -         * Client user credentials are saved in extended attributes.
> -         */
> -        s->ctx.fs_sm = SM_MAPPED;
> -        s->ctx.xops = mapped_xattr_ops;
> -    } else if (!strcmp(fse->security_model, "none")) {
> -        /*
> -         * Files on the fileserver are set to QEMU credentials.
> -         */
> -        s->ctx.fs_sm = SM_NONE;
> -        s->ctx.xops = none_xattr_ops;
> -    } else {
> -        fprintf(stderr, "Default to security_model=none. You may want"
> -                " enable advanced security model using "
> -                "security option:\n\t security_model=passthrough\n\t "
> -                "security_model=mapped\n");
> -        s->ctx.fs_sm = SM_NONE;
> -        s->ctx.xops = none_xattr_ops;
> +    /* security models is needed only for local fs driver */
> +    if (!strcmp(fse->fsdriver, "local")) {
> +        if (!strcmp(fse->security_model, "passthrough")) {
> +            /* Files on the Fileserver set to client user credentials */
> +            s->ctx.fs_sm = SM_PASSTHROUGH;
> +            s->ctx.xops = passthrough_xattr_ops;
> +        } else if (!strcmp(fse->security_model, "mapped")) {
> +            /* Files on the fileserver are set to QEMU credentials.
> +            * Client user credentials are saved in extended attributes.
> +            */
> +            s->ctx.fs_sm = SM_MAPPED;
> +            s->ctx.xops = mapped_xattr_ops;
> +        } else if (!strcmp(fse->security_model, "none")) {
> +            /*
> +            * Files on the fileserver are set to QEMU credentials.
> +            */
> +            s->ctx.fs_sm = SM_NONE;
> +            s->ctx.xops = none_xattr_ops;
> +        } else {
> +            fprintf(stderr, "Invalid security_model %s specified.\n"
> +                    "Available security models are:\t "
> +                    "passthrough,mapped or none\n", fse->security_model);
> +            exit(1);
> +        }
>      }
> 
>      s->ctx.cache_flags = fse->cache_flags;
> diff --git a/vl.c b/vl.c
> index 6760e39..a961fa3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2795,6 +2795,7 @@ int main(int argc, char **argv, char **envp)
>                  QemuOpts *fsdev;
>                  QemuOpts *device;
>                  const char *cache;
> +                const char *fsdriver;
> 
>                  olist = qemu_find_opts("virtfs");
>                  if (!olist) {
> @@ -2809,13 +2810,26 @@ int main(int argc, char **argv, char **envp)
> 
>                  if (qemu_opt_get(opts, "fstype") == NULL ||
>                          qemu_opt_get(opts, "mount_tag") == NULL ||
> -                        qemu_opt_get(opts, "path") == NULL ||
> -                        qemu_opt_get(opts, "security_model") == NULL) {
> +                        qemu_opt_get(opts, "path") == NULL) {
>                      fprintf(stderr, "Usage: -virtfs fstype,path=/share_path/,"
> -                            "security_model=[mapped|passthrough|none],"
> +
>                          "{security_model=[mapped|passthrough|none]},"

That should be
                         [security_model=....]                             


>                              "mount_tag=tag.\n");
>                      exit(1);
>                  }
> +                fsdriver = qemu_opt_get(opts, "fstype");
> +                /* security model is mandatory for local fs driver */
> +                if (!strcmp(fsdriver,"local") &&
> +                                !qemu_opt_get(opts,"security_model")) {
> +                    fprintf(stderr, "security model not specified for local"
> +                                   " fs driver\n");
> +                    exit(1);
> +                }
> +                if (strcmp(fsdriver,"local") &&
> +                                qemu_opt_get(opts,"security_model")) {
> +                    fprintf(stderr, "security model is not needed for %s"
> +                                   " fs driver\n", fsdriver);
> +                    exit(1);
> +                }
> 
>                  fsdev = qemu_opts_create(qemu_find_opts("fsdev"),
>                                           qemu_opt_get(opts, "mount_tag"), 1);

Also needs a documentation update.

-aneesh