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@xxxxxxxxx> --- 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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list