[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Fri, Sep 26, 2014 at 10:32:08AM +0200, Maxime Leroy wrote:
On Thu, Sep 25, 2014 at 11:45 AM, Martin Kletzander <mkletzan 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>


Take that discussed generality into account.  What if there's another
implementation that we'll want to cover with the smem device (be it
qemu or another hypervisor) that allows MSI without any server
configuration.  It won't make sense then.

[..]
+    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.


It is not.  Users can start the server themselves with the listening
socket residing in /var/lib/libvirt/shmem-<name>-sock and it will just
work.  I'm trying to keep that possibility available for the users.

+        /*
+         * 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 ?


Simply, because I forgot about that, thanks for catching that, I'll
remove it ;)

Martin

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]