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

[Ján Tomko]

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 <address@hidden>
> ---
> 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

signature.asc
Description: PGP signature