On Thu, Oct 01, 2020 at 14:23:28 +0100, Brian Turek wrote:
> Introduce new 'fmode' and 'dmode' options for filesystem.
>
> <filesystem type='mount' accessmode='mapped' fmode='644' dmode='755'>
> <source dir='/path'/>
> <target dir='mount_tag'>
> </filesystem>
>
> These options, when used with accessmode=mapped, set the creation mode
> for newly created files and directories (fmode and dmode, respectively)
> on the host. When not specified, QEMU defaults to 600 for files and 700
> for directories.
>
> Signed-off-by: Brian Turek <brian.turek@xxxxxxxxx>
> ---
There is too much stuff going on at the same time in this commit. Split
such changes in the following way:
> docs/formatdomain.rst | 12 ++++
> docs/schemas/domaincommon.rng | 16 +++++
> src/conf/domain_conf.c | 43 +++++++++++++
> src/conf/domain_conf.h | 2 +
XML, schema, parser and docs should be separate.
> src/qemu/qemu_capabilities.c | 4 ++
> src/qemu/qemu_capabilities.h | 2 +
> .../caps_2.10.0.aarch64.xml | 2 +
> .../caps_2.10.0.ppc64.xml | 2 +
> .../caps_2.10.0.s390x.xml | 2 +
> .../caps_2.10.0.x86_64.xml | 2 +
> .../caps_2.11.0.s390x.xml | 2 +
> .../caps_2.11.0.x86_64.xml | 2 +
> .../caps_2.12.0.aarch64.xml | 2 +
> .../caps_2.12.0.ppc64.xml | 2 +
> .../caps_2.12.0.s390x.xml | 2 +
> .../caps_2.12.0.x86_64.xml | 2 +
> .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 +
> .../caps_3.0.0.riscv32.xml | 2 +
> .../caps_3.0.0.riscv64.xml | 2 +
> .../qemucapabilitiesdata/caps_3.0.0.s390x.xml | 2 +
> .../caps_3.0.0.x86_64.xml | 2 +
> .../qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 2 +
> .../caps_3.1.0.x86_64.xml | 2 +
> .../caps_4.0.0.aarch64.xml | 2 +
> .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 +
> .../caps_4.0.0.riscv32.xml | 2 +
> .../caps_4.0.0.riscv64.xml | 2 +
> .../qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 +
> .../caps_4.0.0.x86_64.xml | 2 +
> .../caps_4.1.0.x86_64.xml | 2 +
> .../caps_4.2.0.aarch64.xml | 2 +
> .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 2 +
> .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 2 +
> .../caps_4.2.0.x86_64.xml | 2 +
> .../caps_5.0.0.aarch64.xml | 2 +
> .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 2 +
> .../caps_5.0.0.riscv64.xml | 2 +
> .../caps_5.0.0.x86_64.xml | 2 +
> .../caps_5.1.0.x86_64.xml | 2 +
> .../caps_5.2.0.x86_64.xml | 2 +
qemu_capabilities changes and the corresponding XML update should be
separate.
> src/qemu/qemu_command.c | 6 ++
> src/qemu/qemu_validate.c | 26 ++++++++
> .../virtio-9p-fmodedmode.x86_64-latest.args | 48 +++++++++++++++
> .../qemuxml2argvdata/virtio-9p-fmodedmode.xml | 58 ++++++++++++++++++
> .../virtio-9p-fmodedmode.x86_64-latest.xml | 61 +++++++++++++++++++
> tests/qemuxml2xmltest.c | 1 +
Here you have to consider what is worth separating.
> 46 files changed, 347 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/virtio-9p-fmodedmode.x86_64-latest.args
> create mode 100644 tests/qemuxml2argvdata/virtio-9p-fmodedmode.xml
> create mode 100644 tests/qemuxml2xmloutdata/virtio-9p-fmodedmode.x86_64-latest.xml
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index f3cf9e1fb3..a98bf4948f 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -3062,6 +3062,12 @@ A directory on the host that can be accessed directly from the guest.
> <target dir='/import/from/host'/>
> <readonly/>
> </filesystem>
> + <filesystem type='mount' accessmode='mapped' fmode='644' dmode='755'>
> + <driver type='path'/>
> + <source dir='/export/to/guest'/>
> + <target dir='/import/from/host'/>
> + <readonly/>
> + </filesystem>
> <filesystem type='file' accessmode='passthrough'>
> <driver type='loop' format='raw'/>
> <driver type='path' wrpolicy='immediate'/>
> @@ -3141,6 +3147,12 @@ A directory on the host that can be accessed directly from the guest.
> "virtio-non-transitional", or "virtio". See `Virtio transitional
> devices <#elementsVirtioTransitional>`__ for more details.
>
> + :since:`Since 6.9.0`, the filesystem element has two optional attributes
> + ``fmode`` and ``dmode``. These two attributes control the creation mode for
> + files and directories when used with the ``mapped`` value for ``accessmode``
> + (:since:`requires QEMU 2.10` ). If not specified, files are created witha
the since statement here is unusual
> + mode ``600`` and directories with mode ``700``.
I'd specify that they are created with a mode chosen by the hypervisor
since this may be different for other hypervisor drivers.
> +
> The filesystem element has an optional attribute ``multidevs`` 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 (
[...]
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a91dbd4aa9..5c3d71f31e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
[...]
> @@ -11535,6 +11537,40 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
> def->accessmode = VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH;
> }
>
> + fmode = virXMLPropString(node, "fmode");
> + if (fmode) {
> + if (def->accessmode != VIR_DOMAIN_FS_ACCESSMODE_MAPPED) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("fmode must be used with accessmode=mapped"));
> + goto error;
> + }
We usually put validation of semantics into the validation code rather
than having it in the XML parser.
> +
> + if (virStrToLong_uip(fmode, NULL, 10, &def->fmode) < 0) {
Parsing these as decimal values ('10' as the third argument) is wrong
for file mode, since the number represents an octal value.
In this scenario I can use '999' as fmode which will only be rejeceted
by qemu. Note that XML validation is not mandatory for VM XMLs
(historical reasons), so it can't be relied upon.
Either force octal mode or keep them as strings.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("invalid fmode: '%s'"), fmode);
> + goto error;
> + }
> + } else {
> + def->fmode = 0;
This is default, we always 0 init allocated structs.
> + }
> +
> + dmode = virXMLPropString(node, "dmode");
> + if (dmode) {
> + if (def->accessmode != VIR_DOMAIN_FS_ACCESSMODE_MAPPED) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("dmode must be used with accessmode=mapped"));
> + goto error;
> + }
> +
> + if (virStrToLong_uip(dmode, NULL, 10, &def->dmode) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("invalid dmode: '%s'"), dmode);
> + goto error;
> + }
> + } else {
> + def->dmode = 0;
> + }
> +
> model = virXMLPropString(node, "model");
> if (model) {
> if ((def->model = virDomainFSModelTypeFromString(model)) < 0 ||
> @@ -26123,6 +26159,13 @@ virDomainFSDefFormat(virBufferPtr buf,
> }
> if (def->multidevs)
> virBufferAsprintf(buf, " multidevs='%s'", multidevs);
> +
> + if (def->fmode)
> + virBufferAsprintf(buf, " fmode='%03d'", def->fmode);
> +
> + if (def->dmode)
> + virBufferAsprintf(buf, " dmode='%03d'", def->dmode);
> +
> virBufferAddLit(buf, ">\n");
>
> virBufferAdjustIndent(buf, 2);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8f1662aae0..fabc8c2e56 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -849,6 +849,8 @@ struct _virDomainFSDef {
> int wrpolicy; /* enum virDomainFSWrpolicy */
> int format; /* virStorageFileFormat */
> int model; /* virDomainFSModel */
> + unsigned int fmode;
> + unsigned int dmode;
> int multidevs; /* virDomainFSMultidevs */
> unsigned long long usage; /* in bytes */
> virStorageSourcePtr src;
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 5dcfcd574d..b66fc65444 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -600,6 +600,8 @@ VIR_ENUM_IMPL(virQEMUCaps,
>
> /* 380 */
> "usb-host.hostdevice",
> + "fsdev.fmode",
> + "fsdev.dmode",
Were these introduced at the same time? If yes you don't need two
separate capabilities.
[...]
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index a212605579..37a1fa9528 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -3530,6 +3530,22 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
> return -1;
> }
>
> + if (fs->fmode &&
fmode is an integer, please use a explicit != 0 check for those.
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_FMODE))
> + {
This brace style doesn't conform to our coding style. The opening brace
should be at the same line as the last line of the condition.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("fmode is not supported with this QEMU binary"));
> + return -1;
> + }
> +
> + if (fs->dmode &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_DMODE))
> + {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("dmode is not supported with this QEMU binary"));
> + return -1;
> + }
Same as above.
> +
> switch ((virDomainFSDriverType) fs->fsdriver) {
> case VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT:
> case VIR_DOMAIN_FS_DRIVER_TYPE_PATH:
> @@ -3591,6 +3607,16 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
> _("virtiofs does not support multidevs"));
> return -1;
> }
> + if (fs->fmode) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("virtiofs does not support fmode"));
> + return -1;
> + }
> + if (fs->dmode) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("virtiofs does not support dmode"));
> + return -1;
> + }
IMO you can put both in one condition with one error message mentioning
both dmode/fmode.
> if (qemuValidateDomainDefVirtioFSSharedMemory(def) < 0)
> return -1;
> break;
