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

Re: [libvirt] [PATCH v1 2/6] conf: Parse and format shmem device XML



On Fri, Aug 22, 2014 at 12:47:01PM +0200, Maxime Leroy wrote:
This patch adds configuration support for the shmem device
as described in the schema in the previous patch.

Signed-off-by: Maxime Leroy <maxime leroy 6wind com>
---
src/conf/domain_conf.c   | 249 ++++++++++++++++++++++++++++++++++++++++++++++-
src/conf/domain_conf.h   |  41 ++++++++
src/libvirt_private.syms |   2 +
3 files changed, 291 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9557020..08d653a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
[...]
@@ -9462,6 +9502,135 @@ virDomainNVRAMDefParseXML(xmlNodePtr node,
    return NULL;
}

+static int
+virDomainIvshmemDefParseXML(xmlNodePtr node,
+                            xmlXPathContextPtr ctxt,
+                            virDomainIvshmemDefPtr def)
+{
+    char *ioeventfd = NULL;
+    char *vectors = NULL;
+    xmlNodePtr cur;
+    xmlNodePtr save = ctxt->node;
+    int ret;
+
+    cur = node->children;
+    while (cur != NULL) {
+        if (cur->type == XML_ELEMENT_NODE) {
+            if (xmlStrEqual(cur->name, BAD_CAST "server")) {
+                def->server.enabled = true;
+                if (!(def->server.path = virXMLPropString(cur, "path"))) {
+                    virReportError(VIR_ERR_XML_ERROR, "%s",
+                                   _("cannot parse <server> 'path' attribute"));
+                    goto error;
+                }
+            } else if (xmlStrEqual(cur->name, BAD_CAST "size")) {
+                if (virDomainParseScaledValue("./size[1]", ctxt,
+                                              &def->size, 1,
+                                              ULLONG_MAX, true) < 0) {
+                    virReportError(VIR_ERR_XML_ERROR, "%s",
+                                   _("cannot parse <size> attribute"));
+                    goto error;
+                }

This parsing using loop over children does not readably (and in this
case neither reliably) check whether required options are present.
Currently it parses <shmem name="asdf"/> as valid, but not specifying
the size is probably not what you want on qemu command-line ... [1]

+            } else if (xmlStrEqual(cur->name, BAD_CAST "msi")) {
+                def->msi.enabled = true;
+                vectors = virXMLPropString(cur, "vectors");
+                ioeventfd = virXMLPropString(cur, "ioeventfd");
+
+                if (vectors &&
+                    virStrToLong_ui(vectors, NULL, 10, &def->msi.vectors) < 0) {

Use *_uip() if you don't want to accept negative values.

+                    virReportError(VIR_ERR_XML_ERROR,
+                                   _("cannot parse <msi> 'vectors' attribute '%s'"),
+                                   vectors);
+                    goto error;
+                }
+                if (ioeventfd &&
+                    (def->msi.ioeventfd =
+                     virTristateSwitchTypeFromString(ioeventfd)) <= 0) {
+                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                                   _("cannot parse <msi> 'ioeventfd' mode '%s'"),
+                                   ioeventfd);
+                    goto error;
+                }
+            }
+        }
+        cur = cur->next;
+    }
+
+    /* 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 an ivshmem server"));
+        goto error;
+    }
+
+    /* size should be a power of two */
+    if (def->size & (def->size-1)) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("shmem size should be a power of two for ivshmem model"));
+      goto error;
+    }
+
+    ret = 0;
+ cleanup:
+    ctxt->node = save;
+    VIR_FREE(ioeventfd);
+    VIR_FREE(vectors);
+    return ret;
+
+ error:
+    ret = -1;
+    goto cleanup;
+}
+
+static virDomainShmemDefPtr
+virDomainShmemDefParseXML(xmlNodePtr node,
+                          xmlXPathContextPtr ctxt,
+                          unsigned int flags)
+{
+    char *model = virXMLPropString(node, "model");
+    virDomainShmemDefPtr def;
+
+    if (VIR_ALLOC(def) < 0)
+        return NULL;
+
+    if (model) {
+        if ((def->model == virDomainShmemModelTypeFromString(model)) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("Unknown <shmem> model '%s'"), model);
+            goto error;
+        }
+    } else
+        def->model = VIR_DOMAIN_SHMEM_MODEL_IVSHMEM;
+

As I said in 1/6 (with which this can be merged, so we have support in
schema and parsing code together), the model can be completely dropped.

[...]
@@ -16828,6 +17020,56 @@ static int virDomainPanicDefFormat(virBufferPtr buf,
    return 0;
}

+static int virDomainIvshmemDefFormat(virBufferPtr buf,
+                                     virDomainIvshmemDefPtr def)
+{
+    if (def->server.enabled)
+        virBufferAsprintf(buf, "<server path='%s'/>\n",
+                          def->server.path);

One general idea while looking through the code; could we make the
path optional or even better, leave the "server" out somehow and
generalize it even more.  The path could be for example
"/var/run/libvirt/ivshmem-<name>-sock" since we are starting it
anyway, and permissions on that would be easier to set then (whole
path is prepared already).  The name would then be enough to get
either the shmem name or the server socket path.  Question is whether
we can get the information whether server needs to be started from
somewhere else.  E.g. does server make sense with no msi vectors and
without ioeventfd?

+    if (def->size)
+        virBufferAsprintf(buf, "<size unit='M'>%llu</size>\n",
+                          def->size  / (1024 * 1024));
+

If anyone specifies size < 1M, this won't be properly formatted
(another idea for a test case).

Martin

Attachment: signature.asc
Description: Digital signature


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