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

2014-08-28 Thread Martin Kletzander 

On Wed, Aug 27, 2014 at 10:52:02PM +0200, Maxime Leroy wrote:

On Tue, Aug 26, 2014 at 10:42 AM, Martin Kletzander mklet...@redhat.com 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 shmem name=asdf/ 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 shmem name=asdf/ 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, 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.


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, 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).


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

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

2014-08-27 Thread Maxime Leroy 
On Tue, Aug 26, 2014 at 10:42 AM, Martin Kletzander mklet...@redhat.com 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 shmem name=asdf/ 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 shmem name=asdf/ 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, 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.

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, 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).

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

2014-08-27 Thread Maxime Leroy 
On Tue, Aug 26, 2014 at 11:02 AM, Wang Rui moon.wang...@huawei.com 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

2014-08-26 Thread Martin Kletzander 

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.le...@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,
+

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

2014-08-26 Thread Wang Rui 
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 maxime.le...@6wind.com
 ---

 +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 shmem 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,
 +   _(shmem 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, shmem name='%s' model='%s'\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