Re: [PATCH 1/1] conf: qemu 9pfs: add 'multidevs' option

["Ján Tomko" <jtomko@redhat.com>]

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

On a Tuesday in 2020, Christian Schoenebeck wrote:
>Introduce new 'multidevs' option for filesystem.
>
>  <filesystem type='mount' accessmode='mapped' multidevs='remap'>

I don't like the 'multidevs' name, but cannot think of anything
beter.

'collisions' maybe?

>    <source dir='/path'/>
>    <target dir='mount_tag'>
>  </filesystem>
>
>This option prevents misbheaviours on guest if a 9pfs export
>contains multiple devices, due to the potential file ID collisions
>this otherwise may cause.
>
>Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>---
> docs/formatdomain.html.in     | 47 ++++++++++++++++++++++++++++++++++-
> docs/schemas/domaincommon.rng | 10 ++++++++
> src/conf/domain_conf.c        | 30 ++++++++++++++++++++++
> src/conf/domain_conf.h        | 13 ++++++++++
> src/qemu/qemu_command.c       |  7 ++++++
> 5 files changed, 106 insertions(+), 1 deletion(-)

Please split the XML changes from the qemu driver changes.

Also missing:
* qemu_capabilities addition
* qemuDomainDeviceDefValidateFS in qemu_domain.c - check for the capability,
   reject this setting for virtiofs
* qemuxml2xmltest addition
* qemuxml2argvtest addition

(no changes required for virschematest - it checks all the XML files in
the directories used by the above tests against the schema)

>
>diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>index 594146009d..13c506988b 100644
>--- a/docs/formatdomain.html.in
>+++ b/docs/formatdomain.html.in
>@@ -3967,7 +3967,7 @@
>     &lt;source name='my-vm-template'/&gt;
>     &lt;target dir='/'/&gt;
>   &lt;/filesystem&gt;
>-  &lt;filesystem type='mount' accessmode='passthrough'&gt;
>+  &lt;filesystem type='mount' accessmode='passthrough' multidevs='remap'&gt;
>     &lt;driver type='path' wrpolicy='immediate'/&gt;
>     &lt;source dir='/export/to/guest'/&gt;
>     &lt;target dir='/import/from/host'/&gt;
>@@ -4084,13 +4084,58 @@
>         </dd>
>         </dl>
>
>+      <p>
>       <span class="since">Since 5.2.0</span>, the filesystem element
>       has an optional attribute <code>model</code> with supported values
>       "virtio-transitional", "virtio-non-transitional", or "virtio".
>       See <a href="#elementsVirtioTransitional">Virtio transitional devices</a>
>       for more details.
>+      </p>
>+

Unrelated change that can be split out.

>+      <p>
>+      The filesystem element has an optional attribute <code>multidevs</code>
>+      which specifies how to deal with a filesystem export containing more than
>+      one device, in order to avoid file ID collisions on guest when using 9pfs
>+      (<span class="since">since 6.2.0, requires QEMU 4.2</span>).
>+      This attribute is not available for virtiofs. The possible values are:
>+      </p>
>+
>+        <dl>
>+        <dt><code>default</code></dt>
>+        <dd>
>+        Use QEMU's default setting (which currently is <code>warn</code>).
>+        </dd>
>+        <dt><code>remap</code></dt>
>+        <dd>
>+        This setting allows guest to access multiple devices per export without
>+        encountering misbehaviours. Inode numbers from host are automatically
>+        remapped on guest to actively prevent file ID collisions if guest
>+        accesses one export containing multiple devices.
>+        </dd>
>+        <dt><code>forbid</code></dt>
>+        <dd>
>+        Only allow to access one device per export by guest. Attempts to access
>+        additional devices on the same export will cause the individual
>+        filesystem access by guest to fail with an error and being logged (once)
>+        as error on host side.
>+        </dd>
>+        <dt><code>warn</code></dt>
>+        <dd>
>+        This setting resembles the behaviour of 9pfs prior to QEMU 4.2, that is
>+        no action is performed to prevent any potential file ID collisions if an
>+        export contains multiple devices, with the only exception: a warning is
>+        logged (once) on host side now. This setting may lead to misbehaviours
>+        on guest side if more than one device is exported per export, due to the
>+        potential file ID collisions this may cause on guest side in that case.
>+        </dd>
>+        </dl>
>+
>       </dd>

>
>+      <p>
>+      The <code>filesystem</code> element may contain the following subelements:
>+      </p>
>+

And so can this one.

>       <dt><code>driver</code></dt>
>       <dd>
>         The optional driver element allows specifying further details
>@@ -25422,6 +25449,9 @@ virDomainFSDefFormat(virBufferPtr buf,
>         virBufferAsprintf(buf, " model='%s'",
>                           virDomainFSModelTypeToString(def->model));
>     }
>+    if (def->multidevs) {
>+        virBufferAsprintf(buf, " multidevs='%s'", multidevs);
>+    }

make syntax-check complains here:
Curly brackets around single-line body:
../src/conf/domain_conf.c:25452-25454:
     if (def->multidevs) {
         virBufferAsprintf(buf, " multidevs='%s'", multidevs);
     }

Jano

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