[Qemu-devel] [PATCH 0/6] fsdev/virtfs: Assorted cleanups and fixes

[Qemu-devel] [PATCH 0/6] fsdev/virtfs: Assorted cleanups and fixes

[Greg Kurz]

Hi,

This series does several things, not necessarily related, but I post
them all together anyway because it will be more convenient for me
to merge them after review, given the little time I can spend on
virtfs maintainership.

Thomas,

Patch 6 supersedes http://patchwork.ozlabs.org/patch/1095472/ but I'd
appreciate your valuable feedback on the other ones as well :)

Cheers,

--
Greg

---

Greg Kurz (6):
      fsdev: Drop unused extern declaration
      fsdev: Drop unused opaque field
      fsdev: Move some types definition to qemu-fsdev.c
      fsdev: Error out when unsupported option is passed
      vl: Deprecate -virtfs_synth
      virtfs: Fix documentation of -fsdev and -virtfs


 fsdev/file-op-9p.h   |    1 -
 fsdev/qemu-fsdev.c   |   97 ++++++++++++++++++++++++++++++++++++++++++++++++--
 fsdev/qemu-fsdev.h   |   25 -------------
 qemu-deprecated.texi |    4 ++
 qemu-options.hx      |   87 ++++++++++++++++++++++++++++++++-------------
 vl.c                 |    5 +++
 6 files changed, 165 insertions(+), 54 deletions(-)

[Qemu-devel] [PATCH 1/6] fsdev: Drop unused extern declaration

[Greg Kurz]

This is a leftover of the handle backend, removed in QEMU 4.0.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fsdev/qemu-fsdev.h |    1 -
 1 file changed, 1 deletion(-)

diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h
index d9716b414492..844159d1e1ff 100644
--- a/fsdev/qemu-fsdev.h
+++ b/fsdev/qemu-fsdev.h
@@ -41,7 +41,6 @@ typedef struct FsDriverListEntry {
 int qemu_fsdev_add(QemuOpts *opts, Error **errp);
 FsDriverEntry *get_fsdev_fsentry(char *id);
 extern FileOperations local_ops;
-extern FileOperations handle_ops;
 extern FileOperations synth_ops;
 extern FileOperations proxy_ops;
 #endif

[Qemu-devel] [PATCH 2/6] fsdev: Drop unused opaque field

[Greg Kurz]

This was introduced along with -fsdev but it never got used.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fsdev/file-op-9p.h |    1 -
 1 file changed, 1 deletion(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 3fa062b39f1b..c757c8099f54 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -147,7 +147,6 @@ struct FileOperations
     int (*renameat)(FsContext *ctx, V9fsPath *olddir, const char *old_name,
                     V9fsPath *newdir, const char *new_name);
     int (*unlinkat)(FsContext *ctx, V9fsPath *dir, const char *name, int flags);
-    void *opaque;
 };
 
 #endif

[Qemu-devel] [PATCH 3/6] fsdev: Move some types definition to qemu-fsdev.c

[Greg Kurz]

It would make sense for these types to be defined in a header file if
we had an API for fsdrivers to register themselves. In practice, we
only have three of them and it is very unlikely we add new ones since
the future of file sharing between host and guest is the upcoming
virtio-fs.

Move the types to qemu-fsdev.c instead since they are only used there.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fsdev/qemu-fsdev.c |   23 +++++++++++++++++++++++
 fsdev/qemu-fsdev.h |   24 ------------------------
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index 54cb36a2124b..e972bd698cf5 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -18,6 +18,29 @@
 #include "qemu/error-report.h"
 #include "qemu/option.h"
 
+/*
+ * A table to store the various file systems and their callback operations.
+ * -----------------
+ * fstype | ops
+ * -----------------
+ *  local | local_ops
+ *  .     |
+ *  .     |
+ *  .     |
+ *  .     |
+ * -----------------
+ *  etc
+ */
+typedef struct FsDriverTable {
+    const char *name;
+    FileOperations *ops;
+} FsDriverTable;
+
+typedef struct FsDriverListEntry {
+    FsDriverEntry fse;
+    QTAILQ_ENTRY(FsDriverListEntry) next;
+} FsDriverListEntry;
+
 static QTAILQ_HEAD(, FsDriverListEntry) fsdriver_entries =
     QTAILQ_HEAD_INITIALIZER(fsdriver_entries);
 
diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h
index 844159d1e1ff..52a53977701a 100644
--- a/fsdev/qemu-fsdev.h
+++ b/fsdev/qemu-fsdev.h
@@ -14,30 +14,6 @@
 #define QEMU_FSDEV_H
 #include "file-op-9p.h"
 
-
-/*
- * A table to store the various file systems and their callback operations.
- * -----------------
- * fstype | ops
- * -----------------
- *  local | local_ops
- *  .     |
- *  .     |
- *  .     |
- *  .     |
- * -----------------
- *  etc
- */
-typedef struct FsDriverTable {
-    const char *name;
-    FileOperations *ops;
-} FsDriverTable;
-
-typedef struct FsDriverListEntry {
-    FsDriverEntry fse;
-    QTAILQ_ENTRY(FsDriverListEntry) next;
-} FsDriverListEntry;
-
 int qemu_fsdev_add(QemuOpts *opts, Error **errp);
 FsDriverEntry *get_fsdev_fsentry(char *id);
 extern FileOperations local_ops;

[Qemu-devel] [PATCH 4/6] fsdev: Error out when unsupported option is passed

[Greg Kurz]

Each fsdriver only supports a subset of the options that can be passed
to -fsdev. Unsupported options are simply ignored. This could cause the
user to erroneously think QEMU has a bug.

Enforce strict checking of supported options for all fsdrivers. This
shouldn't impact libvirt, since it doesn't know about he synth and
proxy fsdrivers.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fsdev/qemu-fsdev.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 71 insertions(+), 3 deletions(-)

diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index e972bd698cf5..077a8c4e2bca 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -34,6 +34,7 @@
 typedef struct FsDriverTable {
     const char *name;
     FileOperations *ops;
+    const char **opts;
 } FsDriverTable;
 
 typedef struct FsDriverListEntry {
@@ -44,12 +45,75 @@ typedef struct FsDriverListEntry {
 static QTAILQ_HEAD(, FsDriverListEntry) fsdriver_entries =
     QTAILQ_HEAD_INITIALIZER(fsdriver_entries);
 
+#define COMMON_FS_DRIVER_OPTIONS "id", "fsdriver", "readonly"
+
 static FsDriverTable FsDrivers[] = {
-    { .name = "local", .ops = &local_ops},
-    { .name = "synth", .ops = &synth_ops},
-    { .name = "proxy", .ops = &proxy_ops},
+    {
+        .name = "local",
+        .ops = &local_ops,
+        .opts = (const char * []) {
+            COMMON_FS_DRIVER_OPTIONS,
+            "security_model",
+            "path",
+            "writeout",
+            "fmode",
+            "dmode",
+            "throttling.bps-total",
+            "throttling.bps-read",
+            "throttling.bps-write",
+            "throttling.iops-total",
+            "throttling.iops-read",
+            "throttling.iops-write",
+            "throttling.bps-total-max",
+            "throttling.bps-read-max",
+            "throttling.bps-write-max",
+            "throttling.iops-total-max",
+            "throttling.iops-read-max",
+            "throttling.iops-write-max",
+            "throttling.bps-total-max-length",
+            "throttling.bps-read-max-length",
+            "throttling.bps-write-max-length",
+            "throttling.iops-total-max-length",
+            "throttling.iops-read-max-length",
+            "throttling.iops-write-max-length",
+            "throttling.iops-size",
+        },
+    },
+    {
+        .name = "synth",
+        .ops = &synth_ops,
+        .opts = (const char * []) {
+            COMMON_FS_DRIVER_OPTIONS,
+        },
+    },
+    {
+        .name = "proxy",
+        .ops = &proxy_ops,
+        .opts = (const char * []) {
+            COMMON_FS_DRIVER_OPTIONS,
+            "socket",
+            "sock_fd",
+            "writeout",
+        },
+    },
 };
 
+static int validate_opt(void *opaque, const char *name, const char *value,
+                        Error **errp)
+{
+    FsDriverTable *drv = opaque;
+    const char **opt;
+
+    for (opt = drv->opts; *opt; opt++) {
+        if (!strcmp(*opt, name)) {
+            return 0;
+        }
+    }
+
+    error_setg(errp, "'%s' is invalid for fsdriver '%s'", name, drv->name);
+    return -1;
+}
+
 int qemu_fsdev_add(QemuOpts *opts, Error **errp)
 {
     int i;
@@ -80,6 +144,10 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
         return -1;
     }
 
+    if (qemu_opt_foreach(opts, validate_opt, &FsDrivers[i], errp)) {
+        return -1;
+    }
+
     fsle = g_malloc0(sizeof(*fsle));
     fsle->fse.fsdev_id = g_strdup(fsdev_id);
     fsle->fse.ops = FsDrivers[i].ops;

[Qemu-devel] [PATCH 5/6] vl: Deprecate -virtfs_synth

[Greg Kurz]

The synth fsdriver never got used for anything else but the
QTest testcase for VirtIO 9P. And even there, QTest directly
uses -fsdev synth and -device virtio-9p-{pci|device}.

Signed-off-by: Greg Kurz <groug@kaod.org>
---

This should be Cc'd to libvir-list@redhat.com according to MAINTAINERS,
but libvirt doesn't know about -virtfs_synth, so I choose to not spam :)
---
 qemu-deprecated.texi |    4 ++++
 qemu-options.hx      |    3 ++-
 vl.c                 |    5 +++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 842e71b11dcc..f0ff065e7dc1 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -72,6 +72,10 @@ backend settings instead of environment variables.  To ease migration to
 the new format, the ``-audiodev-help'' option can be used to convert
 the current values of the environment variables to ``-audiodev'' options.
 
+@subsection -virtfs_synth (since 4.1)
+
+The ``-virtfs_synth'' argument is now deprecated with no replacement.
+
 @section QEMU Machine Protocol (QMP) commands
 
 @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
diff --git a/qemu-options.hx b/qemu-options.hx
index 51802cbb266a..9c5cc2e6bf70 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1368,7 +1368,8 @@ DEF("virtfs_synth", 0, QEMU_OPTION_virtfs_synth,
 STEXI
 @item -virtfs_synth
 @findex -virtfs_synth
-Create synthetic file system image
+Create synthetic file system image. Note that this option is deprecated with
+no replacement.
 ETEXI
 
 DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
diff --git a/vl.c b/vl.c
index d9fea0a11966..c010cb3e98df 100644
--- a/vl.c
+++ b/vl.c
@@ -3507,6 +3507,11 @@ int main(int argc, char **argv, char **envp)
                 QemuOpts *fsdev;
                 QemuOpts *device;
 
+                warn_report("The -virtfs_synth option is deprecated and will "
+                            "be removed soon. If the -virtfs_synth option is "
+                            "still useful for you, please send a mail to "
+                            "qemu-devel@nongnu.org with your usecase.");
+
                 fsdev = qemu_opts_create(qemu_find_opts("fsdev"), "v_synth",
                                          1, NULL);
                 if (!fsdev) {

[Qemu-devel] [PATCH 6/6] virtfs: Fix documentation of -fsdev and -virtfs

[Greg Kurz]

This fixes several things:
- add "id" description to -virtfs documentation
- split the description into several lines in both usage and documentation
  for accurateness and clarity
- add documentation and usage of the synth fsdriver
- add "throttling.*" description to -fsdev local
- add some missing periods

Buglink: https://bugs.launchpad.net/qemu/+bug/1581976
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 qemu-options.hx |   84 +++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 60 insertions(+), 24 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 9c5cc2e6bf70..975342dfbd66 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1232,26 +1232,35 @@ the write back by pressing @key{C-a s} (@pxref{disk_images}).
 ETEXI
 
 DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev,
-    "-fsdev fsdriver,id=id[,path=path,][security_model={mapped-xattr|mapped-file|passthrough|none}]\n"
-    " [,writeout=immediate][,readonly][,socket=socket|sock_fd=sock_fd][,fmode=fmode][,dmode=dmode]\n"
+    "-fsdev local,id=id,path=path,security_model=mapped-xattr|mapped-file|passthrough|none\n"
+    " [,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n"
     " [[,throttling.bps-total=b]|[[,throttling.bps-read=r][,throttling.bps-write=w]]]\n"
     " [[,throttling.iops-total=i]|[[,throttling.iops-read=r][,throttling.iops-write=w]]]\n"
     " [[,throttling.bps-total-max=bm]|[[,throttling.bps-read-max=rm][,throttling.bps-write-max=wm]]]\n"
     " [[,throttling.iops-total-max=im]|[[,throttling.iops-read-max=irm][,throttling.iops-write-max=iwm]]]\n"
-    " [[,throttling.iops-size=is]]\n",
+    " [[,throttling.iops-size=is]]\n"
+    "-fsdev proxy,id=id,socket=socket[,writeout=immediate][,readonly]\n"
+    "-fsdev proxy,id=id,sock_fd=sock_fd[,writeout=immediate][,readonly]\n"
+    "-fsdev synth,id=id\n",
     QEMU_ARCH_ALL)
 
 STEXI
 
-@item -fsdev @var{fsdriver},id=@var{id},path=@var{path},[security_model=@var{security_model}][,writeout=@var{writeout}][,readonly][,socket=@var{socket}|sock_fd=@var{sock_fd}][,fmode=@var{fmode}][,dmode=@var{dmode}]
+@item -fsdev local,id=@var{id},path=@var{path},security_model=@var{security_model} [,writeout=@var{writeout}][,readonly][,fmode=@var{fmode}][,dmode=@var{dmode}] [,throttling.@var{option}=@var{value}[,throttling.@var{option}=@var{value}[,...]]]
+@itemx -fsdev proxy,id=@var{id},socket=@var{socket}[,writeout=@var{writeout}][,readonly]
+@itemx -fsdev proxy,id=@var{id},sock_fd=@var{sock_fd}[,writeout=@var{writeout}][,readonly]
+@itemx -fsdev synth,id=@var{id}[,readonly]
 @findex -fsdev
 Define a new file system device. Valid options are:
 @table @option
-@item @var{fsdriver}
-This option specifies the fs driver backend to use.
-Currently "local" and "proxy" file system drivers are supported.
+@item local
+Accesses to the filesystem are done by QEMU.
+@item proxy
+Accesses to the filesystem are done by virtfs-proxy-helper(1).
+@item synth
+Synthetic filesystem, only used by QTests.
 @item id=@var{id}
-Specifies identifier for this device
+Specifies identifier for this device.
 @item path=@var{path}
 Specifies the export path for the file system device. Files under
 this path will be available to the 9p client on the guest.
@@ -1279,17 +1288,33 @@ Enables exporting 9p share as a readonly mount for guests. By default
 read-write access is given.
 @item socket=@var{socket}
 Enables proxy filesystem driver to use passed socket file for communicating
-with virtfs-proxy-helper
+with virtfs-proxy-helper(1).
 @item sock_fd=@var{sock_fd}
 Enables proxy filesystem driver to use passed socket descriptor for
-communicating with virtfs-proxy-helper. Usually a helper like libvirt
-will create socketpair and pass one of the fds as sock_fd
+communicating with virtfs-proxy-helper(1). Usually a helper like libvirt
+will create socketpair and pass one of the fds as sock_fd.
 @item fmode=@var{fmode}
 Specifies the default mode for newly created files on the host. Works only
 with security models "mapped-xattr" and "mapped-file".
 @item dmode=@var{dmode}
 Specifies the default mode for newly created directories on the host. Works
 only with security models "mapped-xattr" and "mapped-file".
+@item throttling.bps-total=@var{b},throttling.bps-read=@var{r},throttling.bps-write=@var{w}
+Specify bandwidth throttling limits in bytes per second, either for all request
+types or for reads or writes only.
+@item throttling.bps-total-max=@var{bm},bps-read-max=@var{rm},bps-write-max=@var{wm}
+Specify bursts in bytes per second, either for all request types or for reads
+or writes only.  Bursts allow the guest I/O to spike above the limit
+temporarily.
+@item throttling.iops-total=@var{i},throttling.iops-read=@var{r}, throttling.iops-write=@var{w}
+Specify request rate limits in requests per second, either for all request
+types or for reads or writes only.
+@item throttling.iops-total-max=@var{im},throttling.iops-read-max=@var{irm}, throttling.iops-write-max=@var{iwm}
+Specify bursts in requests per second, either for all request types or for reads
+or writes only.  Bursts allow the guest I/O to spike above the limit temporarily.
+@item throttling.iops-size=@var{is}
+Let every @var{is} bytes of a request count as a new request for iops
+throttling purposes.
 @end table
 
 -fsdev option is used along with -device driver "virtio-9p-pci".
@@ -1297,30 +1322,39 @@ only with security models "mapped-xattr" and "mapped-file".
 Options for virtio-9p-pci driver are:
 @table @option
 @item fsdev=@var{id}
-Specifies the id value specified along with -fsdev option
+Specifies the id value specified along with -fsdev option.
 @item mount_tag=@var{mount_tag}
-Specifies the tag name to be used by the guest to mount this export point
+Specifies the tag name to be used by the guest to mount this export point.
 @end table
 
 ETEXI
 
 DEF("virtfs", HAS_ARG, QEMU_OPTION_virtfs,
-    "-virtfs local,path=path,mount_tag=tag,security_model=[mapped-xattr|mapped-file|passthrough|none]\n"
-    "        [,id=id][,writeout=immediate][,readonly][,socket=socket|sock_fd=sock_fd][,fmode=fmode][,dmode=dmode]\n",
+    "-virtfs local,path=path,mount_tag=tag,security_model=mapped-xattr|mapped-file|passthrough|none\n"
+    "        [,id=id][,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n"
+    "-virtfs proxy,mount_tag=tag,socket=socket[,id=id][,writeout=immediate][,readonly]\n"
+    "-virtfs proxy,mount_tag=tag,sock_fd=sock_fd[,id=id][,writeout=immediate][,readonly]\n"
+    "-virtfs synth,mount_tag=tag[,id=id][,readonly]\n",
     QEMU_ARCH_ALL)
 
 STEXI
 
-@item -virtfs @var{fsdriver}[,path=@var{path}],mount_tag=@var{mount_tag}[,security_model=@var{security_model}][,writeout=@var{writeout}][,readonly][,socket=@var{socket}|sock_fd=@var{sock_fd}][,fmode=@var{fmode}][,dmode=@var{dmode}]
+@item -virtfs local,path=@var{path},mount_tag=@var{mount_tag} ,security_model=@var{security_model}[,writeout=@var{writeout}][,readonly] [,fmode=@var{fmode}][,dmode=@var{dmode}]
+@itemx -virtfs proxy,socket=@var{socket},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly]
+@itemx -virtfs proxy,sock_fd=@var{sock_fd},mount_tag=@var{mount_tag} [,writeout=@var{writeout}][,readonly]
+@itemx -virtfs synth,mount_tag=@var{mount_tag}
 @findex -virtfs
 
-The general form of a Virtual File system pass-through options are:
+Define a new filesystem device and expose it to the guest using a virtio-9p-device. The general form of a Virtual File system pass-through options are:
 @table @option
-@item @var{fsdriver}
-This option specifies the fs driver backend to use.
-Currently "local" and "proxy" file system drivers are supported.
+@item local
+Accesses to the filesystem are done by QEMU.
+@item proxy
+Accesses to the filesystem are done by virtfs-proxy-helper(1).
+@item synth
+Synthetic filesystem, only used by QTests.
 @item id=@var{id}
-Specifies identifier for this device
+Specifies identifier for the filesystem device
 @item path=@var{path}
 Specifies the export path for the file system device. Files under
 this path will be available to the 9p client on the guest.
@@ -1348,17 +1382,19 @@ Enables exporting 9p share as a readonly mount for guests. By default
 read-write access is given.
 @item socket=@var{socket}
 Enables proxy filesystem driver to use passed socket file for
-communicating with virtfs-proxy-helper. Usually a helper like libvirt
-will create socketpair and pass one of the fds as sock_fd
+communicating with virtfs-proxy-helper(1). Usually a helper like libvirt
+will create socketpair and pass one of the fds as sock_fd.
 @item sock_fd
 Enables proxy filesystem driver to use passed 'sock_fd' as the socket
-descriptor for interfacing with virtfs-proxy-helper
+descriptor for interfacing with virtfs-proxy-helper(1).
 @item fmode=@var{fmode}
 Specifies the default mode for newly created files on the host. Works only
 with security models "mapped-xattr" and "mapped-file".
 @item dmode=@var{dmode}
 Specifies the default mode for newly created directories on the host. Works
 only with security models "mapped-xattr" and "mapped-file".
+@item mount_tag=@var{mount_tag}
+Specifies the tag name to be used by the guest to mount this export point.
 @end table
 ETEXI
 

Re: [Qemu-devel] [PATCH 1/6] fsdev: Drop unused extern declaration

[Thomas Huth]

On 07/05/2019 10.44, Greg Kurz wrote:
> This is a leftover of the handle backend, removed in QEMU 4.0.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  fsdev/qemu-fsdev.h |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h
> index d9716b414492..844159d1e1ff 100644
> --- a/fsdev/qemu-fsdev.h
> +++ b/fsdev/qemu-fsdev.h
> @@ -41,7 +41,6 @@ typedef struct FsDriverListEntry {
>  int qemu_fsdev_add(QemuOpts *opts, Error **errp);
>  FsDriverEntry *get_fsdev_fsentry(char *id);
>  extern FileOperations local_ops;
> -extern FileOperations handle_ops;
>  extern FileOperations synth_ops;
>  extern FileOperations proxy_ops;
>  #endif
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 2/6] fsdev: Drop unused opaque field

[Thomas Huth]

On 07/05/2019 10.44, Greg Kurz wrote:
> This was introduced along with -fsdev but it never got used.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  fsdev/file-op-9p.h |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 3fa062b39f1b..c757c8099f54 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -147,7 +147,6 @@ struct FileOperations
>      int (*renameat)(FsContext *ctx, V9fsPath *olddir, const char *old_name,
>                      V9fsPath *newdir, const char *new_name);
>      int (*unlinkat)(FsContext *ctx, V9fsPath *dir, const char *name, int flags);
> -    void *opaque;
>  };
>  
>  #endif
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 5/6] vl: Deprecate -virtfs_synth

[Thomas Huth]

On 07/05/2019 10.45, Greg Kurz wrote:
> The synth fsdriver never got used for anything else but the
> QTest testcase for VirtIO 9P. And even there, QTest directly
> uses -fsdev synth and -device virtio-9p-{pci|device}.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 
> This should be Cc'd to libvir-list@redhat.com according to MAINTAINERS,
> but libvirt doesn't know about -virtfs_synth, so I choose to not spam :)
> ---
>  qemu-deprecated.texi |    4 ++++
>  qemu-options.hx      |    3 ++-
>  vl.c                 |    5 +++++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 842e71b11dcc..f0ff065e7dc1 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -72,6 +72,10 @@ backend settings instead of environment variables.  To ease migration to
>  the new format, the ``-audiodev-help'' option can be used to convert
>  the current values of the environment variables to ``-audiodev'' options.
>  
> +@subsection -virtfs_synth (since 4.1)
> +
> +The ``-virtfs_synth'' argument is now deprecated with no replacement.
> +
>  @section QEMU Machine Protocol (QMP) commands
>  
>  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 51802cbb266a..9c5cc2e6bf70 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1368,7 +1368,8 @@ DEF("virtfs_synth", 0, QEMU_OPTION_virtfs_synth,
>  STEXI
>  @item -virtfs_synth
>  @findex -virtfs_synth
> -Create synthetic file system image
> +Create synthetic file system image. Note that this option is deprecated with
> +no replacement.
>  ETEXI
>  
>  DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
> diff --git a/vl.c b/vl.c
> index d9fea0a11966..c010cb3e98df 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3507,6 +3507,11 @@ int main(int argc, char **argv, char **envp)
>                  QemuOpts *fsdev;
>                  QemuOpts *device;
>  
> +                warn_report("The -virtfs_synth option is deprecated and will "
> +                            "be removed soon. If the -virtfs_synth option is "
> +                            "still useful for you, please send a mail to "
> +                            "qemu-devel@nongnu.org with your usecase.");
> +
>                  fsdev = qemu_opts_create(qemu_find_opts("fsdev"), "v_synth",
>                                           1, NULL);
>                  if (!fsdev) {

Do you plan to only deprecate the -virtfs_synth option, or also "-fsdev
synth", i.e. the whole "synth" driver? In the first case, I think you
should point the users to use "-fsdev synth" instead of saying "with no
replacement". In the second case, I think you should declare "-fsdev
synth" in the documentation, too.

 Thomas

Re: [Qemu-devel] [PATCH 3/6] fsdev: Move some types definition to qemu-fsdev.c

[Thomas Huth]

On 07/05/2019 10.45, Greg Kurz wrote:
> It would make sense for these types to be defined in a header file if
> we had an API for fsdrivers to register themselves. In practice, we
> only have three of them and it is very unlikely we add new ones since
> the future of file sharing between host and guest is the upcoming
> virtio-fs.
> 
> Move the types to qemu-fsdev.c instead since they are only used there.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  fsdev/qemu-fsdev.c |   23 +++++++++++++++++++++++
>  fsdev/qemu-fsdev.h |   24 ------------------------
>  2 files changed, 23 insertions(+), 24 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 5/6] vl: Deprecate -virtfs_synth

[Greg Kurz]

On Wed, 8 May 2019 10:26:53 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 07/05/2019 10.45, Greg Kurz wrote:
> > The synth fsdriver never got used for anything else but the
> > QTest testcase for VirtIO 9P. And even there, QTest directly
> > uses -fsdev synth and -device virtio-9p-{pci|device}.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > 
> > This should be Cc'd to libvir-list@redhat.com according to MAINTAINERS,
> > but libvirt doesn't know about -virtfs_synth, so I choose to not spam :)
> > ---
> >  qemu-deprecated.texi |    4 ++++
> >  qemu-options.hx      |    3 ++-
> >  vl.c                 |    5 +++++
> >  3 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > index 842e71b11dcc..f0ff065e7dc1 100644
> > --- a/qemu-deprecated.texi
> > +++ b/qemu-deprecated.texi
> > @@ -72,6 +72,10 @@ backend settings instead of environment variables.  To ease migration to
> >  the new format, the ``-audiodev-help'' option can be used to convert
> >  the current values of the environment variables to ``-audiodev'' options.
> >  
> > +@subsection -virtfs_synth (since 4.1)
> > +
> > +The ``-virtfs_synth'' argument is now deprecated with no replacement.
> > +
> >  @section QEMU Machine Protocol (QMP) commands
> >  
> >  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 51802cbb266a..9c5cc2e6bf70 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1368,7 +1368,8 @@ DEF("virtfs_synth", 0, QEMU_OPTION_virtfs_synth,
> >  STEXI
> >  @item -virtfs_synth
> >  @findex -virtfs_synth
> > -Create synthetic file system image
> > +Create synthetic file system image. Note that this option is deprecated with
> > +no replacement.
> >  ETEXI
> >  
> >  DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
> > diff --git a/vl.c b/vl.c
> > index d9fea0a11966..c010cb3e98df 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3507,6 +3507,11 @@ int main(int argc, char **argv, char **envp)
> >                  QemuOpts *fsdev;
> >                  QemuOpts *device;
> >  
> > +                warn_report("The -virtfs_synth option is deprecated and will "
> > +                            "be removed soon. If the -virtfs_synth option is "
> > +                            "still useful for you, please send a mail to "
> > +                            "qemu-devel@nongnu.org with your usecase.");
> > +
> >                  fsdev = qemu_opts_create(qemu_find_opts("fsdev"), "v_synth",
> >                                           1, NULL);
> >                  if (!fsdev) {  
> 
> Do you plan to only deprecate the -virtfs_synth option, or also "-fsdev
> synth", i.e. the whole "synth" driver? In the first case, I think you
> should point the users to use "-fsdev synth" instead of saying "with no
> replacement".

The plan is to remove -virtfs_synth only. You're right, I'll point users
to use "-fsdev synth" and "-device virtio-9p" instead.

> In the second case, I think you should declare "-fsdev
> synth" in the documentation, too.
> 
>  Thomas

Re: [Qemu-devel] [PATCH 6/6] virtfs: Fix documentation of -fsdev and -virtfs

[Thomas Huth]

On 07/05/2019 10.45, Greg Kurz wrote:
> This fixes several things:
> - add "id" description to -virtfs documentation
> - split the description into several lines in both usage and documentation
>   for accurateness and clarity
> - add documentation and usage of the synth fsdriver
> - add "throttling.*" description to -fsdev local
> - add some missing periods
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1581976
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  qemu-options.hx |   84 +++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 60 insertions(+), 24 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9c5cc2e6bf70..975342dfbd66 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1232,26 +1232,35 @@ the write back by pressing @key{C-a s} (@pxref{disk_images}).
>  ETEXI
>  
>  DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev,
> -    "-fsdev fsdriver,id=id[,path=path,][security_model={mapped-xattr|mapped-file|passthrough|none}]\n"
> -    " [,writeout=immediate][,readonly][,socket=socket|sock_fd=sock_fd][,fmode=fmode][,dmode=dmode]\n"
> +    "-fsdev local,id=id,path=path,security_model=mapped-xattr|mapped-file|passthrough|none\n"
> +    " [,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n"
>      " [[,throttling.bps-total=b]|[[,throttling.bps-read=r][,throttling.bps-write=w]]]\n"
>      " [[,throttling.iops-total=i]|[[,throttling.iops-read=r][,throttling.iops-write=w]]]\n"
>      " [[,throttling.bps-total-max=bm]|[[,throttling.bps-read-max=rm][,throttling.bps-write-max=wm]]]\n"
>      " [[,throttling.iops-total-max=im]|[[,throttling.iops-read-max=irm][,throttling.iops-write-max=iwm]]]\n"
> -    " [[,throttling.iops-size=is]]\n",
> +    " [[,throttling.iops-size=is]]\n"
> +    "-fsdev proxy,id=id,socket=socket[,writeout=immediate][,readonly]\n"
> +    "-fsdev proxy,id=id,sock_fd=sock_fd[,writeout=immediate][,readonly]\n"
> +    "-fsdev synth,id=id\n",
>      QEMU_ARCH_ALL)
>  
>  STEXI
>  
> -@item -fsdev @var{fsdriver},id=@var{id},path=@var{path},[security_model=@var{security_model}][,writeout=@var{writeout}][,readonly][,socket=@var{socket}|sock_fd=@var{sock_fd}][,fmode=@var{fmode}][,dmode=@var{dmode}]
> +@item -fsdev local,id=@var{id},path=@var{path},security_model=@var{security_model} [,writeout=@var{writeout}][,readonly][,fmode=@var{fmode}][,dmode=@var{dmode}] [,throttling.@var{option}=@var{value}[,throttling.@var{option}=@var{value}[,...]]]
> +@itemx -fsdev proxy,id=@var{id},socket=@var{socket}[,writeout=@var{writeout}][,readonly]
> +@itemx -fsdev proxy,id=@var{id},sock_fd=@var{sock_fd}[,writeout=@var{writeout}][,readonly]
> +@itemx -fsdev synth,id=@var{id}[,readonly]
>  @findex -fsdev
>  Define a new file system device. Valid options are:
>  @table @option
> -@item @var{fsdriver}
> -This option specifies the fs driver backend to use.
> -Currently "local" and "proxy" file system drivers are supported.
> +@item local
> +Accesses to the filesystem are done by QEMU.
> +@item proxy
> +Accesses to the filesystem are done by virtfs-proxy-helper(1).
> +@item synth
> +Synthetic filesystem, only used by QTests.
>  @item id=@var{id}
> -Specifies identifier for this device
> +Specifies identifier for this device.
>  @item path=@var{path}
>  Specifies the export path for the file system device. Files under
>  this path will be available to the 9p client on the guest.
> @@ -1279,17 +1288,33 @@ Enables exporting 9p share as a readonly mount for guests. By default
>  read-write access is given.
>  @item socket=@var{socket}
>  Enables proxy filesystem driver to use passed socket file for communicating
> -with virtfs-proxy-helper
> +with virtfs-proxy-helper(1).

Why did you add a "(1)" after each virtfs-proxy-helper?

... apart from that, the modifications look fine to me (but as mentioned
earlier, I'm not an expert in this area, so not sure whether that counts
;-))

 Thomas

Re: [Qemu-devel] [PATCH 4/6] fsdev: Error out when unsupported option is passed

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]

On 5/7/19 3:45 AM, Greg Kurz wrote:
> Each fsdriver only supports a subset of the options that can be passed
> to -fsdev. Unsupported options are simply ignored. This could cause the
> user to erroneously think QEMU has a bug.
> 
> Enforce strict checking of supported options for all fsdrivers. This
> shouldn't impact libvirt, since it doesn't know about he synth and

s/he/the/

> proxy fsdrivers.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  fsdev/qemu-fsdev.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 71 insertions(+), 3 deletions(-)
> 

> 
> +#define COMMON_FS_DRIVER_OPTIONS "id", "fsdriver", "readonly"
> +
>  static FsDriverTable FsDrivers[] = {
> -    { .name = "local", .ops = &local_ops},
> -    { .name = "synth", .ops = &synth_ops},
> -    { .name = "proxy", .ops = &proxy_ops},
> +    {
> +        .name = "local",
> +        .ops = &local_ops,
> +        .opts = (const char * []) {
> +            COMMON_FS_DRIVER_OPTIONS,
> +            "security_model",


> +static int validate_opt(void *opaque, const char *name, const char *value,
> +                        Error **errp)
> +{
> +    FsDriverTable *drv = opaque;
> +    const char **opt;
> +
> +    for (opt = drv->opts; *opt; opt++) {
> +        if (!strcmp(*opt, name)) {
> +            return 0;
> +        }
> +    }
> +
> +    error_setg(errp, "'%s' is invalid for fsdriver '%s'", name, drv->name);
> +    return -1;
> +}

When we ever reach command-line QAPIfication, this might go away. In the
meantime, this is an improvement.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 6/6] virtfs: Fix documentation of -fsdev and -virtfs

[Greg Kurz]

On Wed, 8 May 2019 17:54:42 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 07/05/2019 10.45, Greg Kurz wrote:
> > This fixes several things:
> > - add "id" description to -virtfs documentation
> > - split the description into several lines in both usage and documentation
> >   for accurateness and clarity
> > - add documentation and usage of the synth fsdriver
> > - add "throttling.*" description to -fsdev local
> > - add some missing periods
> > 
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1581976
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  qemu-options.hx |   84 +++++++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 60 insertions(+), 24 deletions(-)
> > 
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 9c5cc2e6bf70..975342dfbd66 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1232,26 +1232,35 @@ the write back by pressing @key{C-a s} (@pxref{disk_images}).
> >  ETEXI
> >  
> >  DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev,
> > -    "-fsdev fsdriver,id=id[,path=path,][security_model={mapped-xattr|mapped-file|passthrough|none}]\n"
> > -    " [,writeout=immediate][,readonly][,socket=socket|sock_fd=sock_fd][,fmode=fmode][,dmode=dmode]\n"
> > +    "-fsdev local,id=id,path=path,security_model=mapped-xattr|mapped-file|passthrough|none\n"
> > +    " [,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n"
> >      " [[,throttling.bps-total=b]|[[,throttling.bps-read=r][,throttling.bps-write=w]]]\n"
> >      " [[,throttling.iops-total=i]|[[,throttling.iops-read=r][,throttling.iops-write=w]]]\n"
> >      " [[,throttling.bps-total-max=bm]|[[,throttling.bps-read-max=rm][,throttling.bps-write-max=wm]]]\n"
> >      " [[,throttling.iops-total-max=im]|[[,throttling.iops-read-max=irm][,throttling.iops-write-max=iwm]]]\n"
> > -    " [[,throttling.iops-size=is]]\n",
> > +    " [[,throttling.iops-size=is]]\n"
> > +    "-fsdev proxy,id=id,socket=socket[,writeout=immediate][,readonly]\n"
> > +    "-fsdev proxy,id=id,sock_fd=sock_fd[,writeout=immediate][,readonly]\n"
> > +    "-fsdev synth,id=id\n",
> >      QEMU_ARCH_ALL)
> >  
> >  STEXI
> >  
> > -@item -fsdev @var{fsdriver},id=@var{id},path=@var{path},[security_model=@var{security_model}][,writeout=@var{writeout}][,readonly][,socket=@var{socket}|sock_fd=@var{sock_fd}][,fmode=@var{fmode}][,dmode=@var{dmode}]
> > +@item -fsdev local,id=@var{id},path=@var{path},security_model=@var{security_model} [,writeout=@var{writeout}][,readonly][,fmode=@var{fmode}][,dmode=@var{dmode}] [,throttling.@var{option}=@var{value}[,throttling.@var{option}=@var{value}[,...]]]
> > +@itemx -fsdev proxy,id=@var{id},socket=@var{socket}[,writeout=@var{writeout}][,readonly]
> > +@itemx -fsdev proxy,id=@var{id},sock_fd=@var{sock_fd}[,writeout=@var{writeout}][,readonly]
> > +@itemx -fsdev synth,id=@var{id}[,readonly]
> >  @findex -fsdev
> >  Define a new file system device. Valid options are:
> >  @table @option
> > -@item @var{fsdriver}
> > -This option specifies the fs driver backend to use.
> > -Currently "local" and "proxy" file system drivers are supported.
> > +@item local
> > +Accesses to the filesystem are done by QEMU.
> > +@item proxy
> > +Accesses to the filesystem are done by virtfs-proxy-helper(1).
> > +@item synth
> > +Synthetic filesystem, only used by QTests.
> >  @item id=@var{id}
> > -Specifies identifier for this device
> > +Specifies identifier for this device.
> >  @item path=@var{path}
> >  Specifies the export path for the file system device. Files under
> >  this path will be available to the 9p client on the guest.
> > @@ -1279,17 +1288,33 @@ Enables exporting 9p share as a readonly mount for guests. By default
> >  read-write access is given.
> >  @item socket=@var{socket}
> >  Enables proxy filesystem driver to use passed socket file for communicating
> > -with virtfs-proxy-helper
> > +with virtfs-proxy-helper(1).  
> 
> Why did you add a "(1)" after each virtfs-proxy-helper?
> 

Oops forgot to mention that in the changelog... We have a manual page for the
virtfs-proxy-helper command, and IIUC this is the way for a manual page to
reference another one. Makes sense ?

> ... apart from that, the modifications look fine to me (but as mentioned
> earlier, I'm not an expert in this area, so not sure whether that counts
> ;-))
> 

I'm confident about the content, so if this looks fine to you, I'm good :)

>  Thomas

Re: [Qemu-devel] [PATCH 1/6] fsdev: Drop unused extern declaration

[Greg Kurz]

On Tue, 7 May 2019 12:15:16 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 07/05/2019 10.44, Greg Kurz wrote:
> > This is a leftover of the handle backend, removed in QEMU 4.0.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  fsdev/qemu-fsdev.h |    1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h
> > index d9716b414492..844159d1e1ff 100644
> > --- a/fsdev/qemu-fsdev.h
> > +++ b/fsdev/qemu-fsdev.h
> > @@ -41,7 +41,6 @@ typedef struct FsDriverListEntry {
> >  int qemu_fsdev_add(QemuOpts *opts, Error **errp);
> >  FsDriverEntry *get_fsdev_fsentry(char *id);
> >  extern FileOperations local_ops;
> > -extern FileOperations handle_ops;
> >  extern FileOperations synth_ops;
> >  extern FileOperations proxy_ops;
> >  #endif
> >   
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Applied to https://github.com/gkurz/qemu/commits/9p-next 

Cheers,

--
Greg

Re: [Qemu-devel] [PATCH 2/6] fsdev: Drop unused opaque field

[Greg Kurz]

On Tue, 7 May 2019 12:24:42 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 07/05/2019 10.44, Greg Kurz wrote:
> > This was introduced along with -fsdev but it never got used.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  fsdev/file-op-9p.h |    1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> > index 3fa062b39f1b..c757c8099f54 100644
> > --- a/fsdev/file-op-9p.h
> > +++ b/fsdev/file-op-9p.h
> > @@ -147,7 +147,6 @@ struct FileOperations
> >      int (*renameat)(FsContext *ctx, V9fsPath *olddir, const char *old_name,
> >                      V9fsPath *newdir, const char *new_name);
> >      int (*unlinkat)(FsContext *ctx, V9fsPath *dir, const char *name, int flags);
> > -    void *opaque;
> >  };
> >  
> >  #endif
> >   
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>


Applied to https://github.com/gkurz/qemu/commits/9p-next 

Cheers,

--
Greg

Re: [Qemu-devel] [PATCH 3/6] fsdev: Move some types definition to qemu-fsdev.c

[Greg Kurz]

On Wed, 8 May 2019 10:28:03 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 07/05/2019 10.45, Greg Kurz wrote:
> > It would make sense for these types to be defined in a header file if
> > we had an API for fsdrivers to register themselves. In practice, we
> > only have three of them and it is very unlikely we add new ones since
> > the future of file sharing between host and guest is the upcoming
> > virtio-fs.
> > 
> > Move the types to qemu-fsdev.c instead since they are only used there.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  fsdev/qemu-fsdev.c |   23 +++++++++++++++++++++++
> >  fsdev/qemu-fsdev.h |   24 ------------------------
> >  2 files changed, 23 insertions(+), 24 deletions(-)  
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Applied to https://github.com/gkurz/qemu/commits/9p-next 

Cheers,

--
Greg

Re: [Qemu-devel] [PATCH 4/6] fsdev: Error out when unsupported option is passed

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]

On Wed, 8 May 2019 11:23:46 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 5/7/19 3:45 AM, Greg Kurz wrote:
> > Each fsdriver only supports a subset of the options that can be passed
> > to -fsdev. Unsupported options are simply ignored. This could cause the
> > user to erroneously think QEMU has a bug.
> > 
> > Enforce strict checking of supported options for all fsdrivers. This
> > shouldn't impact libvirt, since it doesn't know about he synth and  
> 
> s/he/the/
> 
> > proxy fsdrivers.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  fsdev/qemu-fsdev.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 71 insertions(+), 3 deletions(-)
> >   
> 
> > 
> > +#define COMMON_FS_DRIVER_OPTIONS "id", "fsdriver", "readonly"
> > +
> >  static FsDriverTable FsDrivers[] = {
> > -    { .name = "local", .ops = &local_ops},
> > -    { .name = "synth", .ops = &synth_ops},
> > -    { .name = "proxy", .ops = &proxy_ops},
> > +    {
> > +        .name = "local",
> > +        .ops = &local_ops,
> > +        .opts = (const char * []) {
> > +            COMMON_FS_DRIVER_OPTIONS,
> > +            "security_model",  
> 
> 
> > +static int validate_opt(void *opaque, const char *name, const char *value,
> > +                        Error **errp)
> > +{
> > +    FsDriverTable *drv = opaque;
> > +    const char **opt;
> > +
> > +    for (opt = drv->opts; *opt; opt++) {
> > +        if (!strcmp(*opt, name)) {
> > +            return 0;
> > +        }
> > +    }
> > +
> > +    error_setg(errp, "'%s' is invalid for fsdriver '%s'", name, drv->name);
> > +    return -1;
> > +}  
> 
> When we ever reach command-line QAPIfication, this might go away. In the
> meantime, this is an improvement.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Fixed the typo in the changelog and applied to https://github.com/gkurz/qemu/commits/9p-next 

Cheers,

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 6/6] virtfs: Fix documentation of -fsdev and -virtfs

[Thomas Huth]

On 09/05/2019 15.18, Greg Kurz wrote:
> On Wed, 8 May 2019 17:54:42 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 07/05/2019 10.45, Greg Kurz wrote:
>>> This fixes several things:
>>> - add "id" description to -virtfs documentation
>>> - split the description into several lines in both usage and documentation
>>>   for accurateness and clarity
>>> - add documentation and usage of the synth fsdriver
>>> - add "throttling.*" description to -fsdev local
>>> - add some missing periods
>>>
>>> Buglink: https://bugs.launchpad.net/qemu/+bug/1581976
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>>  qemu-options.hx |   84 +++++++++++++++++++++++++++++++++++++++----------------
>>>  1 file changed, 60 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>> index 9c5cc2e6bf70..975342dfbd66 100644
>>> --- a/qemu-options.hx
>>> +++ b/qemu-options.hx
>>> @@ -1232,26 +1232,35 @@ the write back by pressing @key{C-a s} (@pxref{disk_images}).
>>>  ETEXI
>>>  
>>>  DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev,
>>> -    "-fsdev fsdriver,id=id[,path=path,][security_model={mapped-xattr|mapped-file|passthrough|none}]\n"
>>> -    " [,writeout=immediate][,readonly][,socket=socket|sock_fd=sock_fd][,fmode=fmode][,dmode=dmode]\n"
>>> +    "-fsdev local,id=id,path=path,security_model=mapped-xattr|mapped-file|passthrough|none\n"
>>> +    " [,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n"
>>>      " [[,throttling.bps-total=b]|[[,throttling.bps-read=r][,throttling.bps-write=w]]]\n"
>>>      " [[,throttling.iops-total=i]|[[,throttling.iops-read=r][,throttling.iops-write=w]]]\n"
>>>      " [[,throttling.bps-total-max=bm]|[[,throttling.bps-read-max=rm][,throttling.bps-write-max=wm]]]\n"
>>>      " [[,throttling.iops-total-max=im]|[[,throttling.iops-read-max=irm][,throttling.iops-write-max=iwm]]]\n"
>>> -    " [[,throttling.iops-size=is]]\n",
>>> +    " [[,throttling.iops-size=is]]\n"
>>> +    "-fsdev proxy,id=id,socket=socket[,writeout=immediate][,readonly]\n"
>>> +    "-fsdev proxy,id=id,sock_fd=sock_fd[,writeout=immediate][,readonly]\n"
>>> +    "-fsdev synth,id=id\n",
>>>      QEMU_ARCH_ALL)
>>>  
>>>  STEXI
>>>  
>>> -@item -fsdev @var{fsdriver},id=@var{id},path=@var{path},[security_model=@var{security_model}][,writeout=@var{writeout}][,readonly][,socket=@var{socket}|sock_fd=@var{sock_fd}][,fmode=@var{fmode}][,dmode=@var{dmode}]
>>> +@item -fsdev local,id=@var{id},path=@var{path},security_model=@var{security_model} [,writeout=@var{writeout}][,readonly][,fmode=@var{fmode}][,dmode=@var{dmode}] [,throttling.@var{option}=@var{value}[,throttling.@var{option}=@var{value}[,...]]]
>>> +@itemx -fsdev proxy,id=@var{id},socket=@var{socket}[,writeout=@var{writeout}][,readonly]
>>> +@itemx -fsdev proxy,id=@var{id},sock_fd=@var{sock_fd}[,writeout=@var{writeout}][,readonly]
>>> +@itemx -fsdev synth,id=@var{id}[,readonly]
>>>  @findex -fsdev
>>>  Define a new file system device. Valid options are:
>>>  @table @option
>>> -@item @var{fsdriver}
>>> -This option specifies the fs driver backend to use.
>>> -Currently "local" and "proxy" file system drivers are supported.
>>> +@item local
>>> +Accesses to the filesystem are done by QEMU.
>>> +@item proxy
>>> +Accesses to the filesystem are done by virtfs-proxy-helper(1).
>>> +@item synth
>>> +Synthetic filesystem, only used by QTests.
>>>  @item id=@var{id}
>>> -Specifies identifier for this device
>>> +Specifies identifier for this device.
>>>  @item path=@var{path}
>>>  Specifies the export path for the file system device. Files under
>>>  this path will be available to the 9p client on the guest.
>>> @@ -1279,17 +1288,33 @@ Enables exporting 9p share as a readonly mount for guests. By default
>>>  read-write access is given.
>>>  @item socket=@var{socket}
>>>  Enables proxy filesystem driver to use passed socket file for communicating
>>> -with virtfs-proxy-helper
>>> +with virtfs-proxy-helper(1).  
>>
>> Why did you add a "(1)" after each virtfs-proxy-helper?
>>
> 
> Oops forgot to mention that in the changelog... We have a manual page for the
> virtfs-proxy-helper command, and IIUC this is the way for a manual page to
> reference another one. Makes sense ?

Makes sense for the man page ... but it might look a little bit strange
in the qemu-doc.html file? I've got no strong opinion, but I think I'd
rather not include the "(1)" here.

 Thomas

Re: [Qemu-devel] [PATCH 6/6] virtfs: Fix documentation of -fsdev and -virtfs

[Greg Kurz]

On Mon, 13 May 2019 10:39:17 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 09/05/2019 15.18, Greg Kurz wrote:
> > On Wed, 8 May 2019 17:54:42 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 07/05/2019 10.45, Greg Kurz wrote:  
> >>> This fixes several things:
> >>> - add "id" description to -virtfs documentation
> >>> - split the description into several lines in both usage and documentation
> >>>   for accurateness and clarity
> >>> - add documentation and usage of the synth fsdriver
> >>> - add "throttling.*" description to -fsdev local
> >>> - add some missing periods
> >>>
> >>> Buglink: https://bugs.launchpad.net/qemu/+bug/1581976
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>> ---
> >>>  qemu-options.hx |   84 +++++++++++++++++++++++++++++++++++++++----------------
> >>>  1 file changed, 60 insertions(+), 24 deletions(-)
> >>>
> >>> diff --git a/qemu-options.hx b/qemu-options.hx
> >>> index 9c5cc2e6bf70..975342dfbd66 100644
> >>> --- a/qemu-options.hx
> >>> +++ b/qemu-options.hx
> >>> @@ -1232,26 +1232,35 @@ the write back by pressing @key{C-a s} (@pxref{disk_images}).
> >>>  ETEXI
> >>>  
> >>>  DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev,
> >>> -    "-fsdev fsdriver,id=id[,path=path,][security_model={mapped-xattr|mapped-file|passthrough|none}]\n"
> >>> -    " [,writeout=immediate][,readonly][,socket=socket|sock_fd=sock_fd][,fmode=fmode][,dmode=dmode]\n"
> >>> +    "-fsdev local,id=id,path=path,security_model=mapped-xattr|mapped-file|passthrough|none\n"
> >>> +    " [,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n"
> >>>      " [[,throttling.bps-total=b]|[[,throttling.bps-read=r][,throttling.bps-write=w]]]\n"
> >>>      " [[,throttling.iops-total=i]|[[,throttling.iops-read=r][,throttling.iops-write=w]]]\n"
> >>>      " [[,throttling.bps-total-max=bm]|[[,throttling.bps-read-max=rm][,throttling.bps-write-max=wm]]]\n"
> >>>      " [[,throttling.iops-total-max=im]|[[,throttling.iops-read-max=irm][,throttling.iops-write-max=iwm]]]\n"
> >>> -    " [[,throttling.iops-size=is]]\n",
> >>> +    " [[,throttling.iops-size=is]]\n"
> >>> +    "-fsdev proxy,id=id,socket=socket[,writeout=immediate][,readonly]\n"
> >>> +    "-fsdev proxy,id=id,sock_fd=sock_fd[,writeout=immediate][,readonly]\n"
> >>> +    "-fsdev synth,id=id\n",
> >>>      QEMU_ARCH_ALL)
> >>>  
> >>>  STEXI
> >>>  
> >>> -@item -fsdev @var{fsdriver},id=@var{id},path=@var{path},[security_model=@var{security_model}][,writeout=@var{writeout}][,readonly][,socket=@var{socket}|sock_fd=@var{sock_fd}][,fmode=@var{fmode}][,dmode=@var{dmode}]
> >>> +@item -fsdev local,id=@var{id},path=@var{path},security_model=@var{security_model} [,writeout=@var{writeout}][,readonly][,fmode=@var{fmode}][,dmode=@var{dmode}] [,throttling.@var{option}=@var{value}[,throttling.@var{option}=@var{value}[,...]]]
> >>> +@itemx -fsdev proxy,id=@var{id},socket=@var{socket}[,writeout=@var{writeout}][,readonly]
> >>> +@itemx -fsdev proxy,id=@var{id},sock_fd=@var{sock_fd}[,writeout=@var{writeout}][,readonly]
> >>> +@itemx -fsdev synth,id=@var{id}[,readonly]
> >>>  @findex -fsdev
> >>>  Define a new file system device. Valid options are:
> >>>  @table @option
> >>> -@item @var{fsdriver}
> >>> -This option specifies the fs driver backend to use.
> >>> -Currently "local" and "proxy" file system drivers are supported.
> >>> +@item local
> >>> +Accesses to the filesystem are done by QEMU.
> >>> +@item proxy
> >>> +Accesses to the filesystem are done by virtfs-proxy-helper(1).
> >>> +@item synth
> >>> +Synthetic filesystem, only used by QTests.
> >>>  @item id=@var{id}
> >>> -Specifies identifier for this device
> >>> +Specifies identifier for this device.
> >>>  @item path=@var{path}
> >>>  Specifies the export path for the file system device. Files under
> >>>  this path will be available to the 9p client on the guest.
> >>> @@ -1279,17 +1288,33 @@ Enables exporting 9p share as a readonly mount for guests. By default
> >>>  read-write access is given.
> >>>  @item socket=@var{socket}
> >>>  Enables proxy filesystem driver to use passed socket file for communicating
> >>> -with virtfs-proxy-helper
> >>> +with virtfs-proxy-helper(1).    
> >>
> >> Why did you add a "(1)" after each virtfs-proxy-helper?
> >>  
> > 
> > Oops forgot to mention that in the changelog... We have a manual page for the
> > virtfs-proxy-helper command, and IIUC this is the way for a manual page to
> > reference another one. Makes sense ?  
> 
> Makes sense for the man page ... but it might look a little bit strange
> in the qemu-doc.html file? I've got no strong opinion, but I think I'd
> rather not include the "(1)" here.
> 

FWIW, we already have some similar references to manual pages:

$ grep '([1-9])' qemu-doc.html
<p>Note that, by default, GUS shares IRQ(7) with parallel ports and so
QEMU mmap(2) <samp>mem-path</samp>, and accepts common suffixes, eg
<dd><p>is a QEMU user creatable object definition. See the <code>qemu(1)</code> manual
<p>The size syntax is similar to dd(1)&rsquo;s size syntax.
See the <code>qemu(1)</code> manual page for full details of the properties

>  Thomas

Re: [Qemu-devel] [PATCH 6/6] virtfs: Fix documentation of -fsdev and -virtfs

[Thomas Huth]

On 13/05/2019 11.34, Greg Kurz wrote:
> On Mon, 13 May 2019 10:39:17 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 09/05/2019 15.18, Greg Kurz wrote:
>>> On Wed, 8 May 2019 17:54:42 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>   
>>>> On 07/05/2019 10.45, Greg Kurz wrote:  
>>>>> This fixes several things:
>>>>> - add "id" description to -virtfs documentation
>>>>> - split the description into several lines in both usage and documentation
>>>>>   for accurateness and clarity
>>>>> - add documentation and usage of the synth fsdriver
>>>>> - add "throttling.*" description to -fsdev local
>>>>> - add some missing periods
>>>>>
>>>>> Buglink: https://bugs.launchpad.net/qemu/+bug/1581976
>>>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>>>> ---
>>>>>  qemu-options.hx |   84 +++++++++++++++++++++++++++++++++++++++----------------
>>>>>  1 file changed, 60 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>>>> index 9c5cc2e6bf70..975342dfbd66 100644
>>>>> --- a/qemu-options.hx
>>>>> +++ b/qemu-options.hx
>>>>> @@ -1232,26 +1232,35 @@ the write back by pressing @key{C-a s} (@pxref{disk_images}).
>>>>>  ETEXI
>>>>>  
>>>>>  DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev,
>>>>> -    "-fsdev fsdriver,id=id[,path=path,][security_model={mapped-xattr|mapped-file|passthrough|none}]\n"
>>>>> -    " [,writeout=immediate][,readonly][,socket=socket|sock_fd=sock_fd][,fmode=fmode][,dmode=dmode]\n"
>>>>> +    "-fsdev local,id=id,path=path,security_model=mapped-xattr|mapped-file|passthrough|none\n"
>>>>> +    " [,writeout=immediate][,readonly][,fmode=fmode][,dmode=dmode]\n"
>>>>>      " [[,throttling.bps-total=b]|[[,throttling.bps-read=r][,throttling.bps-write=w]]]\n"
>>>>>      " [[,throttling.iops-total=i]|[[,throttling.iops-read=r][,throttling.iops-write=w]]]\n"
>>>>>      " [[,throttling.bps-total-max=bm]|[[,throttling.bps-read-max=rm][,throttling.bps-write-max=wm]]]\n"
>>>>>      " [[,throttling.iops-total-max=im]|[[,throttling.iops-read-max=irm][,throttling.iops-write-max=iwm]]]\n"
>>>>> -    " [[,throttling.iops-size=is]]\n",
>>>>> +    " [[,throttling.iops-size=is]]\n"
>>>>> +    "-fsdev proxy,id=id,socket=socket[,writeout=immediate][,readonly]\n"
>>>>> +    "-fsdev proxy,id=id,sock_fd=sock_fd[,writeout=immediate][,readonly]\n"
>>>>> +    "-fsdev synth,id=id\n",
>>>>>      QEMU_ARCH_ALL)
>>>>>  
>>>>>  STEXI
>>>>>  
>>>>> -@item -fsdev @var{fsdriver},id=@var{id},path=@var{path},[security_model=@var{security_model}][,writeout=@var{writeout}][,readonly][,socket=@var{socket}|sock_fd=@var{sock_fd}][,fmode=@var{fmode}][,dmode=@var{dmode}]
>>>>> +@item -fsdev local,id=@var{id},path=@var{path},security_model=@var{security_model} [,writeout=@var{writeout}][,readonly][,fmode=@var{fmode}][,dmode=@var{dmode}] [,throttling.@var{option}=@var{value}[,throttling.@var{option}=@var{value}[,...]]]
>>>>> +@itemx -fsdev proxy,id=@var{id},socket=@var{socket}[,writeout=@var{writeout}][,readonly]
>>>>> +@itemx -fsdev proxy,id=@var{id},sock_fd=@var{sock_fd}[,writeout=@var{writeout}][,readonly]
>>>>> +@itemx -fsdev synth,id=@var{id}[,readonly]
>>>>>  @findex -fsdev
>>>>>  Define a new file system device. Valid options are:
>>>>>  @table @option
>>>>> -@item @var{fsdriver}
>>>>> -This option specifies the fs driver backend to use.
>>>>> -Currently "local" and "proxy" file system drivers are supported.
>>>>> +@item local
>>>>> +Accesses to the filesystem are done by QEMU.
>>>>> +@item proxy
>>>>> +Accesses to the filesystem are done by virtfs-proxy-helper(1).
>>>>> +@item synth
>>>>> +Synthetic filesystem, only used by QTests.
>>>>>  @item id=@var{id}
>>>>> -Specifies identifier for this device
>>>>> +Specifies identifier for this device.
>>>>>  @item path=@var{path}
>>>>>  Specifies the export path for the file system device. Files under
>>>>>  this path will be available to the 9p client on the guest.
>>>>> @@ -1279,17 +1288,33 @@ Enables exporting 9p share as a readonly mount for guests. By default
>>>>>  read-write access is given.
>>>>>  @item socket=@var{socket}
>>>>>  Enables proxy filesystem driver to use passed socket file for communicating
>>>>> -with virtfs-proxy-helper
>>>>> +with virtfs-proxy-helper(1).    
>>>>
>>>> Why did you add a "(1)" after each virtfs-proxy-helper?
>>>>  
>>>
>>> Oops forgot to mention that in the changelog... We have a manual page for the
>>> virtfs-proxy-helper command, and IIUC this is the way for a manual page to
>>> reference another one. Makes sense ?  
>>
>> Makes sense for the man page ... but it might look a little bit strange
>> in the qemu-doc.html file? I've got no strong opinion, but I think I'd
>> rather not include the "(1)" here.
>>
> 
> FWIW, we already have some similar references to manual pages:
> 
> $ grep '([1-9])' qemu-doc.html
> <p>Note that, by default, GUS shares IRQ(7) with parallel ports and so
> QEMU mmap(2) <samp>mem-path</samp>, and accepts common suffixes, eg
> <dd><p>is a QEMU user creatable object definition. See the <code>qemu(1)</code> manual
> <p>The size syntax is similar to dd(1)&rsquo;s size syntax.
> See the <code>qemu(1)</code> manual page for full details of the properties

Ok, you convinced me.

 Thomas