Re: [libvirt PATCH v4 2/4] qemu: add 'fmode' and 'dmode' options

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Thursday in 2020, Brian Turek wrote:
> Expose QEMU's 9pfs 'fmode' and 'dmode' options via attributes on the
> 'filesystem' node in the domain XML. These options control the creation
> mode of files and directories, respectively, when using
> accessmode=mapped.
>
> Signed-off-by: Brian Turek <brian.turek@xxxxxxxxx>
> ---
> docs/schemas/domaincommon.rng                 | 16 +++++
> src/conf/domain_conf.c                        | 29 +++++++++
> src/conf/domain_conf.h                        |  2 +
> .../qemuxml2argvdata/virtio-9p-createmode.xml | 58 ++++++++++++++++++
> .../virtio-9p-createmode.x86_64-latest.xml    | 61 +++++++++++++++++++
> tests/qemuxml2xmltest.c                       |  1 +
> 6 files changed, 167 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/virtio-9p-createmode.xml
> create mode 100644 tests/qemuxml2xmloutdata/virtio-9p-createmode.x86_64-latest.xml
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 7d4b105981..a0f0eb5a23 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -26,6 +26,12 @@
>     </element>
>   </define>
>
> +  <define name="createMode">
> +    <data type="unsignedInt">
> +      <param name="pattern">0[0-7]{3}|[0-7]{1,3}</param>
> +    </data>
> +  </define>
> +
>   <!--
>       We handle only document defining a domain
>     -->
> @@ -2736,6 +2742,16 @@
>             </choice>
>           </attribute>
>         </optional>
> +        <optional>
> +          <attribute name="fmode">
> +            <ref name="createMode"/>
> +          </attribute>
> +        </optional>
> +        <optional>
> +          <attribute name="dmode">
> +            <ref name="createMode"/>
> +          </attribute>
> +        </optional>
>         <optional>
>           <element name="readonly">
>             <empty/>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 51efeb0e42..57ca2152ea 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11471,6 +11471,8 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
>     g_autofree char *units = NULL;
>     g_autofree char *model = NULL;
>     g_autofree char *multidevs = NULL;
> +    g_autofree char *fmode = NULL;
> +    g_autofree char *dmode = NULL;
>
>     ctxt->node = node;
>
> @@ -11499,6 +11501,26 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
>         def->accessmode = VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH;
>     }
>
> +    fmode = virXMLPropString(node, "fmode");
> +    if (fmode) {
> +        if ((virStrToLong_uip(fmode, NULL, 8, &def->fmode) < 0) ||
> +        (def->fmode > 0777)) {

The parentheses around the two parts joined by || are not necessary,
but they do make the second line look better if indented properly.

(The second line should be indented by four spaces, to match the column
 after the opening parenthesis. I'll do that before pushing)

> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("invalid fmode: '%s'"), fmode);
> +            goto error;
> +        }
> +    }
> +
> +    dmode = virXMLPropString(node, "dmode");
> +    if (dmode) {
> +        if ((virStrToLong_uip(dmode, NULL, 8, &def->dmode) < 0) ||
> +        (def->dmode > 0777)) {

Same here

> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("invalid dmode: '%s'"), dmode);
> +            goto error;
> +        }
> +    }
> +
>     model = virXMLPropString(node, "model");
>     if (model) {
>         if ((def->model = virDomainFSModelTypeFromString(model)) < 0 ||

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature