Re: [libvirt] [PATCH v1 2/6] conf: Parse and format shmem device XML
On Wed, Aug 27, 2014 at 10:52:02PM +0200, Maxime Leroy wrote:
On Tue, Aug 26, 2014 at 10:42 AM, Martin Kletzander wrote:
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.
[...]
This parsing using loop over children does not readably (and in this
case neither reliably) check whether required options are present.
For example, I could use: servernode = virXPathNode("./server", ctxt))
instead of having a loop ?
Yes, sure. I think that the parsing parts using the loops are
some very old code leftovers. Maybe from some SAX parsing, but I
don't remember that, just guessing.
Currently it parses as valid, but not specifying
the size is probably not what you want on qemu command-line ... [1]
Size is an optional parameter of ivshmem in qemu. By default, the size is 4MB.
Thus is valid.
Oh, I didn't know that. That is OK, but it should be mentioned in the
documentation, and tests for that should be included as well.
[...]
+if (vectors &&
+virStrToLong_ui(vectors, NULL, 10, &def->msi.vectors)
< 0) {
Use *_uip() if you don't want to accept negative values.
Ok. Thanks.
[...]
+static int virDomainIvshmemDefFormat(virBufferPtr buf,
+ virDomainIvshmemDefPtr def)
+{
+if (def->server.enabled)
+virBufferAsprintf(buf, "\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--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.
If libvirt starts the ivshmem server, then we can use a default path.
If libvirt did not start the server, then we should still let the path
as an optional field for cases where the user wants to start the
server himself with a custom path.
We can have an optional path field to handle both cases.
I agree.
...
[coming back after few minutes]
...
Maybe you were right the first time. Since we're not starting the
server, the path should be specified all the time. After we add the
support for the server, we can relax the checks for the XML parsing.
That's always possible, but impossible the other way around (i.e. we
cannot make the checks more strict in the future). So I'd say keep
the path mandatory and relax it in thelast patch that adds the support
for start='yes'.
One more suggestion that would help us get the patches earlier (before
the ivshmem-server is packaged in qemu) would be to check whether the
start='' attribute is in the XML and forbid such configuration. And
also relax that check after the support for ivshmem-server is added.
Would you be OK with that? What's you opinion?
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?
The server handles eventfds distribution across ivshmem clients.
These eventfds are used to trigger interrupts (with or without msi) in
the "guests" clients.
So you can imagine starting a server without msi to only trigger
interrupts in the guests.
I would say that the server can be seen as a "support interrupt"
property of the shmem object but calling it server is more explicit.
That explicit naming is what I wanted to get rid of, originally (why I
asked the question above). I'm still thinking about the
generalization of "shared memory" instead of exactly matching one
particular technology+hypervisor (qemu+ivshmem).
+if (def->size)
+virBufferAsprintf(buf, "%llu\n",
+ def->size / (1024 * 1024));
+
If anyone specifies size < 1M, this won't be properly formatted
(another idea for a test case).
Ivshmem in qemu only accepts size in MB or GB.
Well, that should be somehow written in the documentation, then. But
if I'm reading David's patch properly (not that I'm qemu-experienced
or anything), it might be smaller after his patches:
https://lists.gnu.org/archive/html/qemu-devel/2014-08/msg01243.html
We should somehow be able to parse any size in src/conf/domain_conf.c
(global parsing code), but PostParse() functions for QEMU
(src/qemu/qemu_domain.c) should allow only what's possible in QEMU.
Or even better, if we can introspect how qemu parses the size (whether
size=1 means 1 B or 1 MB; or whether it supports size=1K for example),
we can forbid invalid values in qemuBuildCommandLine() based on the
qemu that we'll be running.
Martin
To prevent this error, we should not accept size under 1MB, in
virDomainIvshmemDefParseXML:
if (virDomainParseScaledValue("./size[1]", ctxt,
-
Re: [libvirt] [PATCH v1 2/6] conf: Parse and format shmem device XML
On Tue, Aug 26, 2014 at 11:02 AM, Wang Rui wrote:
> On 2014/8/22 18:47, Maxime Leroy wrote:
>> This patch adds configuration support for the shmem device
>> as described in the schema in the previous patch.
[..]
>> +if (model) {
>> +if ((def->model == virDomainShmemModelTypeFromString(model)) < 0) {
>
> I guess you intend 'def->modle = ...' not 'def->model == ...' .
>
Thanks. Anyway model attribute will be removed.
Maxime
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 2/6] conf: Parse and format shmem device XML
On Tue, Aug 26, 2014 at 10:42 AM, Martin Kletzander wrote:
> 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.
>>
[...]
>
> This parsing using loop over children does not readably (and in this
> case neither reliably) check whether required options are present.
For example, I could use: servernode = virXPathNode("./server", ctxt))
instead of having a loop ?
> Currently it parses as valid, but not specifying
> the size is probably not what you want on qemu command-line ... [1]
Size is an optional parameter of ivshmem in qemu. By default, the size is 4MB.
Thus is valid.
[...]
>> +if (vectors &&
>> +virStrToLong_ui(vectors, NULL, 10, &def->msi.vectors)
>> < 0) {
>
>
> Use *_uip() if you don't want to accept negative values.
>
Ok. Thanks.
[...]
>> +static int virDomainIvshmemDefFormat(virBufferPtr buf,
>> + virDomainIvshmemDefPtr def)
>> +{
>> +if (def->server.enabled)
>> +virBufferAsprintf(buf, "\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--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.
If libvirt starts the ivshmem server, then we can use a default path.
If libvirt did not start the server, then we should still let the path
as an optional field for cases where the user wants to start the
server himself with a custom path.
We can have an optional path field to handle both cases.
> 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?
The server handles eventfds distribution across ivshmem clients.
These eventfds are used to trigger interrupts (with or without msi) in
the "guests" clients.
So you can imagine starting a server without msi to only trigger
interrupts in the guests.
I would say that the server can be seen as a "support interrupt"
property of the shmem object but calling it server is more explicit.
>
>> +if (def->size)
>> +virBufferAsprintf(buf, "%llu\n",
>> + def->size / (1024 * 1024));
>> +
>
>
> If anyone specifies size < 1M, this won't be properly formatted
> (another idea for a test case).
Ivshmem in qemu only accepts size in MB or GB.
To prevent this error, we should not accept size under 1MB, in
virDomainIvshmemDefParseXML:
if (virDomainParseScaledValue("./size[1]", ctxt,
- &def->size, 1,
+ &def->size, 1024x1024,
ULLONG_MAX, true) < 0)
Maxime
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 2/6] conf: Parse and format shmem device XML
On 2014/8/22 18:47, 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
> ---
> +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) {
I guess you intend 'def->modle = ...' not 'def->model == ...' .
> +virReportError(VIR_ERR_XML_ERROR,
> + _("Unknown model '%s'"), model);
> +goto error;
> +}
> +} else
> +def->model = VIR_DOMAIN_SHMEM_MODEL_IVSHMEM;
> +
> +if (!(def->name = virXMLPropString(node, "name"))) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _(" must contain 'name' attribute"));
> +goto error;
> +}
As Martin mentioned, 'name' is not necessary.
> +static int virDomainShmemDefFormat(virBufferPtr buf,
> + virDomainShmemDefPtr def,
> + unsigned int flags)
> +{
> +virBufferAsprintf(buf, "\n",
> + def->name,
> virDomainShmemModelTypeToString(def->model));
Here too, 'name' is not necessary
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
---
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 '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 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 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 'vectors' attribute
'%s'"),
+ vectors);
+goto error;
+}
+if (ioeventfd &&
+(def->msi.ioeventfd =
+ virTristateSwitchTypeFromString(ioeventfd)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("cannot parse '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 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, "\n",
+ d
[libvirt] [PATCH v1 2/6] conf: Parse and format shmem device XML
This patch adds configuration support for the shmem device
as described in the schema in the previous patch.
Signed-off-by: Maxime Leroy
---
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
@@ -234,7 +234,8 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST,
"chr",
"memballoon",
"nvram",
- "rng")
+ "rng",
+ "shmem")
VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST,
"none",
@@ -759,6 +760,9 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState,
VIR_DOMAIN_DISK_MIRROR_STATE_LAST,
"abort",
"pivot")
+VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST,
+ "ivshmem")
+
/* Internal mapping: subset of block job types that can be present in
* XML (remaining types are not two-phase). */
VIR_ENUM_DECL(virDomainBlockJob)
@@ -1692,6 +1696,26 @@ void virDomainWatchdogDefFree(virDomainWatchdogDefPtr
def)
VIR_FREE(def);
}
+void virDomainShmemDefFree(virDomainShmemDefPtr def)
+{
+if (!def)
+return;
+
+switch (def->model) {
+case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM:
+VIR_FREE(def->data.ivshmem.server.path);
+break;
+default:
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected shmem model %d"), def->model);
+}
+
+virDomainDeviceInfoClear(&def->info);
+
+VIR_FREE(def->name);
+VIR_FREE(def);
+}
+
void virDomainVideoDefFree(virDomainVideoDefPtr def)
{
if (!def)
@@ -1893,6 +1917,9 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def)
case VIR_DOMAIN_DEVICE_NVRAM:
virDomainNVRAMDefFree(def->data.nvram);
break;
+case VIR_DOMAIN_DEVICE_SHMEM:
+virDomainShmemDefFree(def->data.shmem);
+break;
case VIR_DOMAIN_DEVICE_LAST:
case VIR_DOMAIN_DEVICE_NONE:
break;
@@ -2134,6 +2161,10 @@ void virDomainDefFree(virDomainDefPtr def)
virDomainRedirFilterDefFree(def->redirfilter);
+for (i = 0; i < def->nshmems; i++)
+virDomainShmemDefFree(def->shmems[i]);
+VIR_FREE(def->shmems);
+
if (def->namespaceData && def->ns.free)
(def->ns.free)(def->namespaceData);
@@ -2568,6 +2599,8 @@ virDomainDeviceGetInfo(virDomainDeviceDefPtr device)
return &device->data.memballoon->info;
case VIR_DOMAIN_DEVICE_NVRAM:
return &device->data.nvram->info;
+case VIR_DOMAIN_DEVICE_SHMEM:
+return &device->data.shmem->info;
case VIR_DOMAIN_DEVICE_RNG:
return &device->data.rng->info;
@@ -2783,6 +2816,12 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
if (cb(def, &device, &def->hubs[i]->info, opaque) < 0)
return -1;
}
+device.type = VIR_DOMAIN_DEVICE_SHMEM;
+for (i = 0; i < def->nshmems; i++) {
+device.data.shmem = def->shmems[i];
+if (cb(def, &device, &def->shmems[i]->info, opaque) < 0)
+return -1;
+}
/* This switch statement is here to trigger compiler warning when adding
* a new device type. When you are adding a new field to the switch you
@@ -2809,6 +2848,7 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
case VIR_DOMAIN_DEVICE_CHR:
case VIR_DOMAIN_DEVICE_MEMBALLOON:
case VIR_DOMAIN_DEVICE_NVRAM:
+case VIR_DOMAIN_DEVICE_SHMEM:
case VIR_DOMAIN_DEVICE_LAST:
case VIR_DOMAIN_DEVICE_RNG:
break;
@@ -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 '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",
+ _("can
