[libvirt] [PATCH v2 1/3] docs, conf, schema: add support for shmem device

[Maxime Leroy]

On Thu, Sep 25, 2014 at 11:45 AM, Martin Kletzander <mkletzan at redhat.com> wrote:
> This patch adds parsing/formatting code as well as documentation for
> shared memory devices.  This will currently be only accessible in QEMU
> using it's ivshmem device, but is designed as generic as possible to
> allow future expansion for other hypervisors.
>
> In the devices section in the domain XML users may specify:
>
> - For shmem device using a server:
>
>  <shmem name='shmem0'>
>    <server path='/tmp/socket-ivshmem0'/>
>    <size unit='M'>32</size>
>    <msi vectors='32' ioeventfd='on'/>
>  </shmem>

I would prefer:

<shmem name='shmem0'>
   <server path='/tmp/socket-ivshmem0'>
      <msi vectors='32' ioeventfd='on'/>
   </server>
   <size unit='M'>32</size>
</shmem>

[..]
> +    if ((server = virXPathNode("./server", ctxt))) {
> +        def->server.enabled = true;
> +
> +        if ((tmp = virXMLPropString(server, "path")))
> +            def->server.path = virFileSanitizePath(tmp);
> +        VIR_FREE(tmp);
> +

The server path is mandatory if the ivshmem server is not started by libvirt.

If the user needs to start manually the ivshmem server, having a
default path doesn't make sense anymore.

> +        /*
> +         * We can always safely remove this check, but until the
> +         * server starting part is in it is better to directly disable
> +         * it rather then just ignoring it.
> +         */
> +        if ((tmp = virXMLPropString(server, "start"))) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("Unsupported start attribute for "
> +                             "shmem server element"));

If an autostart ivshmem server is implemented, as we agree to
implement for the V2 (see
http://www.redhat.com/archives/libvir-list/2014-September/msg00124.html),
there is no need for a "start" attribute in the server node.

Why this 'dead' code ?

> +            goto cleanup;
> +        }
> +    }
> +
> +    if ((msi = virXPathNode("./msi", ctxt))) {
> +        def->msi.enabled = true;
> +
> +        if ((tmp = virXMLPropString(msi, "vectors")) &&
> +            virStrToLong_uip(tmp, NULL, 0, &def->msi.vectors) < 0) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("invalid number of vectors for shmem: '%s'"),
> +                           tmp);
> +            goto cleanup;
> +        }
> +        VIR_FREE(tmp);
> +
> +        if ((tmp = virXMLPropString(msi, "ioeventfd")) &&
> +            (def->msi.ioeventfd = virTristateSwitchTypeFromString(tmp)) <= 0) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("invalid msi ioeventfd setting for shmem: '%s'"),
> +                           tmp);
> +            goto cleanup;
> +        }
> +        VIR_FREE(tmp);
> +    }
> +
> +    /* msi option is only relevant with a server */
> +    if (def->msi.enabled && !def->server.enabled) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("msi option is only supported with a server"));
> +        goto cleanup;

If the msi node is inside the server node, i.e:
    <server path='/tmp/socket-ivshmem0'>
      <msi vectors='32' ioeventfd='on'/>
    </server>
Then, this test can be removed.


Maxime