Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

2014-08-28 Thread Martin Kletzander

On Thu, Aug 28, 2014 at 03:46:47PM +0200, David Marchand wrote:


On 08/28/2014 02:26 PM, David Marchand wrote:



I'm not sure, though, what to do with the first point (race between
libvirt creating the socket to see that it exists and ivshmem
disconnecting).  Maybe libvirt could do this (if QEMU would support
it):

1: try to create the socket (if it exists, continue with 4)

2: connect to the socket

3: if connecting fails, goto 1;

4: if libvirt was the one who created the socket, start the server
and pass the FD of the socket to it

5: start qemu and pass the socket to it (qemu already supports that
with other devices like netdevs, etc.

This would take care of all three points.  No race, no permission
issues, nothing.

What do you think?


- Mmm, I had felt that there could be a race in the socket check, yes.
The LISTEN_FDS support in the server is not that big of a change.
I think I can take care of that.


- Did not think of the other points.
I think there is still some problem with your proposition, I need more
time to think about it (may be some prototyping to be sure).


I had misunderstood something about listen()/connect().
Ok, your proposition looks good to me.

At least for the server, this should be transparent as long as the
server handles LISTEN_FDS env variable or an option to tell it which fd
he should listen on.



Parameter is fine, too, I was probably just thinking about the
LISTEN_FDS stuff too much due to having some trouble implementing it
in libvirtd.


For the ivshmem part in QEMU itself, I think adding a property to
ivshmem pci class should do the trick, will see if I (or Maxime) can do
this.



Great.  One more minor thing, though.  In libvirt, we need to have the
option to know whether QEMU supports that newly added option.  We are
introspecting such things using QMP, so if it's a device property, we
should be able to get that.


Are there any other points concerning the server ?



Not that I know of (yet).  Feel free to Cc me on the patches for the
ivshmem stuff in qemu-devel.

Martin


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

2014-08-28 Thread David Marchand


On 08/28/2014 02:26 PM, David Marchand wrote:



I'm not sure, though, what to do with the first point (race between
libvirt creating the socket to see that it exists and ivshmem
disconnecting).  Maybe libvirt could do this (if QEMU would support
it):

1: try to create the socket (if it exists, continue with 4)

2: connect to the socket

3: if connecting fails, goto 1;

4: if libvirt was the one who created the socket, start the server
and pass the FD of the socket to it

5: start qemu and pass the socket to it (qemu already supports that
with other devices like netdevs, etc.

This would take care of all three points.  No race, no permission
issues, nothing.

What do you think?


- Mmm, I had felt that there could be a race in the socket check, yes.
The LISTEN_FDS support in the server is not that big of a change.
I think I can take care of that.


- Did not think of the other points.
I think there is still some problem with your proposition, I need more
time to think about it (may be some prototyping to be sure).


I had misunderstood something about listen()/connect().
Ok, your proposition looks good to me.

At least for the server, this should be transparent as long as the 
server handles LISTEN_FDS env variable or an option to tell it which fd 
he should listen on.


For the ivshmem part in QEMU itself, I think adding a property to 
ivshmem pci class should do the trick, will see if I (or Maxime) can do 
this.


Are there any other points concerning the server ?


--
David Marchand

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

2014-08-28 Thread David Marchand


On 08/28/2014 12:34 PM, Martin Kletzander wrote:


Great!  Really, and it's even easier than what I thought of.  We just
need to kill the server if we fail to start QEMU after starting the
server.

There is one caveat, though.  There is a race between libvirt checking
whether the socket exists and last domain disconnecting.  We also need
to be sure that the server is running and accepts the connection from
QEMU.  And we need to be able to specify the SELinux context of the
socket before it is created.  Adding SELinux support into
ivshmem-server would be too much, however.

Second and third points could be completely eliminated by the server
accepting LISTEN_FDS environment variable which would say that libvirt
is passing an FD with the socket already opened (libvirt would be sure
QEMU is already connected to the socket, and it would take care of the
permissions).  To see how this works you can have a looks at our
daemon code for libvirtd or virtlock, but even easier would probably
be to check systemd's documentation on socket activation (even though
this has nothing to do with systemd).  I coded it up without systemd
based on:

http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html

and some other pages.  Trying to understand it from libvirt sources
might be an overkill.

I'm not sure, though, what to do with the first point (race between
libvirt creating the socket to see that it exists and ivshmem
disconnecting).  Maybe libvirt could do this (if QEMU would support
it):

1: try to create the socket (if it exists, continue with 4)

2: connect to the socket

3: if connecting fails, goto 1;

4: if libvirt was the one who created the socket, start the server
and pass the FD of the socket to it

5: start qemu and pass the socket to it (qemu already supports that
with other devices like netdevs, etc.

This would take care of all three points.  No race, no permission
issues, nothing.

What do you think?


- Mmm, I had felt that there could be a race in the socket check, yes.
The LISTEN_FDS support in the server is not that big of a change.
I think I can take care of that.


- Did not think of the other points.
I think there is still some problem with your proposition, I need more 
time to think about it (may be some prototyping to be sure).







[snip]


+/**
+ * virStopIvshmemServer:
+ * @shm_name: the name of the shared memory
+ *
+ * Stop an Ivshmem Server
+ *
+ * Returns 0 in case of success or -1 in case of failure.
+ */
+int
+virStopIvshmemServer(const char *shm_name)
+{
+char *ivshmserver_pidbase = NULL;
+pid_t ivshmserver_pid;
+int ret = -1;
+
+/* get pid of the ivshmem server */
+if (!(ivshmserver_pidbase =
virIvshmemServerPidFileBasename(shm_name)))
+goto cleanup;
+if (virPidFileRead(IVSHMEM_STATE_DIR, ivshmserver_pidbase,
+&ivshmserver_pid))
+goto cleanup;
+
+virProcessKill(ivshmserver_pid, SIGTERM);
+virPidFileDelete(IVSHMEM_STATE_DIR, ivshmserver_pidbase);


The pidfile support could be added to the server too, maybe...


I am not sure I understand the point.

The only thing this code does here is retrieve the current pid for the
ivshmem-server, it kills the associated pid then remove the pidfile.

So what do you mean by "pidfile support" ?
Do you suggest that, on exit, the ivshmem-server should remove the
pidfile it first created ?



Disregard this comment, our above idea takes care of anything I might
have meant in here :)


Yes, I thought so, no problem.


Thanks Martin.

--
David Marchand

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

2014-08-28 Thread Martin Kletzander

On Thu, Aug 28, 2014 at 10:34:11AM +0200, David Marchand wrote:

Hello Martin,


On 08/26/2014 11:33 AM, Martin Kletzander wrote:

[Cc'ing David due to some questions/ideas about the ivshmem server]


@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd,
return -1;
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
+
+if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) {
+if (virStartIvshmemServer(dev->name,
ivshmem->server.path,
+  ivshmem->size,
ivshmem->msi.vectors))
+return -1;
+}
}


What if the server is already running?  Either 2 domains have server
start='yes' or the user started it already.  We should not fail in
these cases, I guess.



[snip]



It would be great if the domain with server start='yes' didn't have to
be started first and killed last.  Or if we could just signal the
server (let's say SIGUSR1) that it should unlink the shmem and quit if
no domains are connected.  That way we could just send a signal to the
server whenever any domain is disconnecting (maybe some check that no
domain is being started might be good too ;) ).


Why not.

If we want to automagically start/stop the server, we don't need the
'start' field, but we need a way to find if a server is running for this
shared mem instance.

We can try to bind the unix socket and check if a EADDRINUSE is returned.
  If EADDRINUSE, server is running, nothing to do.
  Else libvirt starts the server with the option "-q" (new option I can
  add which means "quit on last client disconnect")

On stop, libvirt does nothing about the ivshmem-server.

With this, if the server had been started with "-q" (by libvirt or by
the user), then the server will quit on the last disconnect.
If the user did not start the server with -q, it is his reponsibility to
stop it.

The 'start' field can disappear.
No need to send any signal to ivshmem-server.


What do you think of this ?



Great!  Really, and it's even easier than what I thought of.  We just
need to kill the server if we fail to start QEMU after starting the
server.

There is one caveat, though.  There is a race between libvirt checking
whether the socket exists and last domain disconnecting.  We also need
to be sure that the server is running and accepts the connection from
QEMU.  And we need to be able to specify the SELinux context of the
socket before it is created.  Adding SELinux support into
ivshmem-server would be too much, however.

Second and third points could be completely eliminated by the server
accepting LISTEN_FDS environment variable which would say that libvirt
is passing an FD with the socket already opened (libvirt would be sure
QEMU is already connected to the socket, and it would take care of the
permissions).  To see how this works you can have a looks at our
daemon code for libvirtd or virtlock, but even easier would probably
be to check systemd's documentation on socket activation (even though
this has nothing to do with systemd).  I coded it up without systemd
based on:

http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html

and some other pages.  Trying to understand it from libvirt sources
might be an overkill.

I'm not sure, though, what to do with the first point (race between
libvirt creating the socket to see that it exists and ivshmem
disconnecting).  Maybe libvirt could do this (if QEMU would support
it):

1: try to create the socket (if it exists, continue with 4)

2: connect to the socket

3: if connecting fails, goto 1;

4: if libvirt was the one who created the socket, start the server
   and pass the FD of the socket to it

5: start qemu and pass the socket to it (qemu already supports that
   with other devices like netdevs, etc.

This would take care of all three points.  No race, no permission
issues, nothing.

What do you think?



[snip]


+/**
+ * virStopIvshmemServer:
+ * @shm_name: the name of the shared memory
+ *
+ * Stop an Ivshmem Server
+ *
+ * Returns 0 in case of success or -1 in case of failure.
+ */
+int
+virStopIvshmemServer(const char *shm_name)
+{
+char *ivshmserver_pidbase = NULL;
+pid_t ivshmserver_pid;
+int ret = -1;
+
+/* get pid of the ivshmem server */
+if (!(ivshmserver_pidbase =
virIvshmemServerPidFileBasename(shm_name)))
+goto cleanup;
+if (virPidFileRead(IVSHMEM_STATE_DIR, ivshmserver_pidbase,
+&ivshmserver_pid))
+goto cleanup;
+
+virProcessKill(ivshmserver_pid, SIGTERM);
+virPidFileDelete(IVSHMEM_STATE_DIR, ivshmserver_pidbase);


The pidfile support could be added to the server too, maybe...


I am not sure I understand the point.

The only thing this code does here is retrieve the current pid for the
ivshmem-server, it kills the associated pid then remove the pidfile.

So what do you mean by "pidfile support" ?
Do you suggest that, on exit, the ivshmem-server should remove the
pid

Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

2014-08-28 Thread Martin Kletzander

On Thu, Aug 28, 2014 at 02:35:10PM +0800, Wang Rui wrote:

On 2014/8/28 5:20, Maxime Leroy wrote:

On Tue, Aug 26, 2014 at 11:58 AM, Wang Rui  wrote:

On 2014/8/22 18:47, Maxime Leroy wrote:


+# util/virivshmemserver.h
+virStartIvshmemServer;
+virStopIvshmemServer;


I think function name virIvshmemStartServer is better.
So is the stop function.



What about virIvshmemServerStart ?


It looks fine.



Yes, it looks better.


@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd,
 return -1;
 virCommandAddArg(cmd, devstr);
 VIR_FREE(devstr);
+
+if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) {
+if (virStartIvshmemServer(dev->name, ivshmem->server.path,
+  ivshmem->size, ivshmem->msi.vectors))
+return -1;
+}
 }


I'm not sure that calling virStartIvshmemServer in qemuBuildIvshmemCommandLine
is the best way. Maybe qemuBuild*CommandLine() usually only build commandline.



Calling virStartIvshmemServer in qemuProcessStart should be better ?


Looks better, too. But we'd better to wait for other opinions.



It should be started as late as possible and stopped if no client
connects there.  This will need more changes I think, we'll discuss it
with David in the other thread.

Martin


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

2014-08-28 Thread David Marchand

Hello Martin,


On 08/26/2014 11:33 AM, Martin Kletzander wrote:

[Cc'ing David due to some questions/ideas about the ivshmem server]


@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd,
return -1;
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
+
+if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) {
+if (virStartIvshmemServer(dev->name,
ivshmem->server.path,
+  ivshmem->size,
ivshmem->msi.vectors))
+return -1;
+}
}


What if the server is already running?  Either 2 domains have server
start='yes' or the user started it already.  We should not fail in
these cases, I guess.



[snip]



It would be great if the domain with server start='yes' didn't have to
be started first and killed last.  Or if we could just signal the
server (let's say SIGUSR1) that it should unlink the shmem and quit if
no domains are connected.  That way we could just send a signal to the
server whenever any domain is disconnecting (maybe some check that no
domain is being started might be good too ;) ).


Why not.

If we want to automagically start/stop the server, we don't need the 
'start' field, but we need a way to find if a server is running for this 
shared mem instance.


We can try to bind the unix socket and check if a EADDRINUSE is returned.
  If EADDRINUSE, server is running, nothing to do.
  Else libvirt starts the server with the option "-q" (new option I can
  add which means "quit on last client disconnect")

On stop, libvirt does nothing about the ivshmem-server.

With this, if the server had been started with "-q" (by libvirt or by 
the user), then the server will quit on the last disconnect.
If the user did not start the server with -q, it is his reponsibility to 
stop it.


The 'start' field can disappear.
No need to send any signal to ivshmem-server.


What do you think of this ?


[snip]


+/**
+ * virStopIvshmemServer:
+ * @shm_name: the name of the shared memory
+ *
+ * Stop an Ivshmem Server
+ *
+ * Returns 0 in case of success or -1 in case of failure.
+ */
+int
+virStopIvshmemServer(const char *shm_name)
+{
+char *ivshmserver_pidbase = NULL;
+pid_t ivshmserver_pid;
+int ret = -1;
+
+/* get pid of the ivshmem server */
+if (!(ivshmserver_pidbase =
virIvshmemServerPidFileBasename(shm_name)))
+goto cleanup;
+if (virPidFileRead(IVSHMEM_STATE_DIR, ivshmserver_pidbase,
+&ivshmserver_pid))
+goto cleanup;
+
+virProcessKill(ivshmserver_pid, SIGTERM);
+virPidFileDelete(IVSHMEM_STATE_DIR, ivshmserver_pidbase);


The pidfile support could be added to the server too, maybe...


I am not sure I understand the point.

The only thing this code does here is retrieve the current pid for the 
ivshmem-server, it kills the associated pid then remove the pidfile.


So what do you mean by "pidfile support" ?
Do you suggest that, on exit, the ivshmem-server should remove the 
pidfile it first created ?





--
David Marchand

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

2014-08-27 Thread Wang Rui
On 2014/8/28 5:20, Maxime Leroy wrote:
> On Tue, Aug 26, 2014 at 11:58 AM, Wang Rui  wrote:
>> On 2014/8/22 18:47, Maxime Leroy wrote:
>>
>>> +# util/virivshmemserver.h
>>> +virStartIvshmemServer;
>>> +virStopIvshmemServer;
>>
>> I think function name virIvshmemStartServer is better.
>> So is the stop function.
>>
> 
> What about virIvshmemServerStart ?

It looks fine.

>>> @@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd,
>>>  return -1;
>>>  virCommandAddArg(cmd, devstr);
>>>  VIR_FREE(devstr);
>>> +
>>> +if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) {
>>> +if (virStartIvshmemServer(dev->name, ivshmem->server.path,
>>> +  ivshmem->size, 
>>> ivshmem->msi.vectors))
>>> +return -1;
>>> +}
>>>  }
>>
>> I'm not sure that calling virStartIvshmemServer in 
>> qemuBuildIvshmemCommandLine
>> is the best way. Maybe qemuBuild*CommandLine() usually only build 
>> commandline.
>>
> 
> Calling virStartIvshmemServer in qemuProcessStart should be better ?

Looks better, too. But we'd better to wait for other opinions.

> Maxime
> 
> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

2014-08-27 Thread Maxime Leroy
On Tue, Aug 26, 2014 at 11:58 AM, Wang Rui  wrote:
> On 2014/8/22 18:47, Maxime Leroy wrote:
>
>> +# util/virivshmemserver.h
>> +virStartIvshmemServer;
>> +virStopIvshmemServer;
>
> I think function name virIvshmemStartServer is better.
> So is the stop function.
>

What about virIvshmemServerStart ?

>> @@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd,
>>  return -1;
>>  virCommandAddArg(cmd, devstr);
>>  VIR_FREE(devstr);
>> +
>> +if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) {
>> +if (virStartIvshmemServer(dev->name, ivshmem->server.path,
>> +  ivshmem->size, 
>> ivshmem->msi.vectors))
>> +return -1;
>> +}
>>  }
>
> I'm not sure that calling virStartIvshmemServer in qemuBuildIvshmemCommandLine
> is the best way. Maybe qemuBuild*CommandLine() usually only build commandline.
>

Calling virStartIvshmemServer in qemuProcessStart should be better ?

Maxime

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

2014-08-26 Thread Wang Rui
On 2014/8/22 18:47, Maxime Leroy wrote:

> +# util/virivshmemserver.h
> +virStartIvshmemServer;
> +virStopIvshmemServer;

I think function name virIvshmemStartServer is better.
So is the stop function.

> @@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd,
>  return -1;
>  virCommandAddArg(cmd, devstr);
>  VIR_FREE(devstr);
> +
> +if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) {
> +if (virStartIvshmemServer(dev->name, ivshmem->server.path,
> +  ivshmem->size, 
> ivshmem->msi.vectors))
> +return -1;
> +}
>  }

I'm not sure that calling virStartIvshmemServer in qemuBuildIvshmemCommandLine
is the best way. Maybe qemuBuild*CommandLine() usually only build commandline.

>  
>  return 0;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index baa866a..aaf03a3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -67,6 +67,7 @@
>  #include "virstring.h"
>  #include "virhostdev.h"
>  #include "storage/storage_driver.h"
> +#include "virivshmemserver.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> @@ -4684,6 +4685,15 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>  }
>  }
>  
> +/* stop runnning ivshmem server */
> +for (i = 0; i < vm->def->nshmems; i++) {
> +virDomainShmemDefPtr shmem =  vm->def->shmems[i];
> +if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM &&

You can use switch (for extension other shmem in future).


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

2014-08-26 Thread Martin Kletzander

[Cc'ing David due to some questions/ideas about the ivshmem server]

On Fri, Aug 22, 2014 at 12:47:05PM +0200, Maxime Leroy wrote:

With the new start param, we are able to start the
ivshmem-server.

With this xml:

 

32

 

Libvirt will execute an ivshmem-server:

 /usr/bin/ivshmem-server -m ivshmem0 -S /tmp/ivshmem0.sock  \
   -p /var/run/libvirt/ivshmem-server/ivshd-ivshmem0.pid  -n 32

Signed-off-by: Maxime Leroy 
---
configure.ac|   4 +
docs/formatdomain.html.in   |   5 +-
docs/schemas/domaincommon.rng   |   3 +
po/POTFILES.in  |   1 +
src/Makefile.am |   1 +
src/conf/domain_conf.c  |  19 +++-
src/conf/domain_conf.h  |   1 +
src/libvirt_private.syms|   5 +
src/qemu/qemu_command.c |   7 ++
src/qemu/qemu_process.c |  10 ++
src/util/virivshmemserver.c | 141 
src/util/virivshmemserver.h |  28 +
tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml |   2 +-
13 files changed, 223 insertions(+), 4 deletions(-)
create mode 100644 src/util/virivshmemserver.c
create mode 100644 src/util/virivshmemserver.h


[...]

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9fcceae..fe2fc1a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -34,6 +34,7 @@
#include "virarch.h"
#include "virerror.h"
#include "virfile.h"
+#include "virivshmemserver.h"
#include "virnetdev.h"
#include "virstring.h"
#include "virtime.h"
@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd,
return -1;
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
+
+if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) {
+if (virStartIvshmemServer(dev->name, ivshmem->server.path,
+  ivshmem->size, ivshmem->msi.vectors))
+return -1;
+}
}


What if the server is already running?  Either 2 domains have server
start='yes' or the user started it already.  We should not fail in
these cases, I guess.



return 0;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index baa866a..aaf03a3 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -67,6 +67,7 @@
#include "virstring.h"
#include "virhostdev.h"
#include "storage/storage_driver.h"
+#include "virivshmemserver.h"

#define VIR_FROM_THIS VIR_FROM_QEMU

@@ -4684,6 +4685,15 @@ void qemuProcessStop(virQEMUDriverPtr driver,
}
}

+/* stop runnning ivshmem server */
+for (i = 0; i < vm->def->nshmems; i++) {
+virDomainShmemDefPtr shmem =  vm->def->shmems[i];
+if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM &&
+shmem->data.ivshmem.server.start == VIR_TRISTATE_BOOL_YES) {
+virStopIvshmemServer(shmem->name);
+}
+}
+


It would be great if the domain with server start='yes' didn't have to
be started first and killed last.  Or if we could just signal the
server (let's say SIGUSR1) that it should unlink the shmem and quit if
no domains are connected.  That way we could just send a signal to the
server whenever any domain is disconnecting (maybe some check that no
domain is being started might be good too ;) ).


vm->taint = 0;
vm->pid = -1;
virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason);
diff --git a/src/util/virivshmemserver.c b/src/util/virivshmemserver.c
new file mode 100644
index 000..89130b1
--- /dev/null
+++ b/src/util/virivshmemserver.c
@@ -0,0 +1,141 @@
+/*
+ * Copyright 2014 6WIND S.A.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ */
+
+#include 
+#include 
+#include 
+
+#include "virivshmemserver.h"
+#include "vircommand.h"
+#include "viralloc.h"
+#include "virerror.h"
+#include "virlog.h"
+#include "virstring.h"
+#include "virfile.h"
+#include "virpidfile.h"
+#include "virprocess.h"
+#include "configmake.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("util.ivshmemserver")
+
+#define IVSHMEM_STATE_DIR LOCALSTATEDIR "/run/libvirt/ivshmem-server"
+
+static char *virIvshmemServerPidFileBasename(const char *shm_name) {
+char *p

[libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

2014-08-22 Thread Maxime Leroy
With the new start param, we are able to start the
ivshmem-server.

With this xml:

  
 
 32
 
  

Libvirt will execute an ivshmem-server:

  /usr/bin/ivshmem-server -m ivshmem0 -S /tmp/ivshmem0.sock  \
-p /var/run/libvirt/ivshmem-server/ivshd-ivshmem0.pid  -n 32

Signed-off-by: Maxime Leroy 
---
 configure.ac|   4 +
 docs/formatdomain.html.in   |   5 +-
 docs/schemas/domaincommon.rng   |   3 +
 po/POTFILES.in  |   1 +
 src/Makefile.am |   1 +
 src/conf/domain_conf.c  |  19 +++-
 src/conf/domain_conf.h  |   1 +
 src/libvirt_private.syms|   5 +
 src/qemu/qemu_command.c |   7 ++
 src/qemu/qemu_process.c |  10 ++
 src/util/virivshmemserver.c | 141 
 src/util/virivshmemserver.h |  28 +
 tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml |   2 +-
 13 files changed, 223 insertions(+), 4 deletions(-)
 create mode 100644 src/util/virivshmemserver.c
 create mode 100644 src/util/virivshmemserver.h

diff --git a/configure.ac b/configure.ac
index f93c6c2..6b525b9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -431,6 +431,8 @@ AC_PATH_PROG([SCRUB], [scrub], [scrub],
[/sbin:/usr/sbin:/usr/local/sbin:$PATH])
 AC_PATH_PROG([ADDR2LINE], [addr2line], [addr2line],
 [/sbin:/usr/bin:/usr/sbin:/usr/local/sbin:$PATH])
+AC_PATH_PROG([IVSHMEMSERVER], [ivshmem-server], [ivshmem-server],
+   [/usr/bin:/usr/local/bin:$PATH])
 
 AC_DEFINE_UNQUOTED([DMIDECODE],["$DMIDECODE"],
 [Location or name of the dmidecode program])
@@ -463,6 +465,8 @@ AC_DEFINE_UNQUOTED([SCRUB],["$SCRUB"],
 [Location or name of the scrub program (for wiping algorithms)])
 AC_DEFINE_UNQUOTED([ADDR2LINE],["$ADDR2LINE"],
 [Location of addr2line program])
+AC_DEFINE_UNQUOTED([IVSHMEMSERVER], ["$IVSHMEMSERVER"],
+   [Location or name of ivshmem-server program])
 
 dnl Specific dir for HTML output ?
 AC_ARG_WITH([html-dir], [AS_HELP_STRING([--with-html-dir=path],
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index c7644bc..df231d8 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -5462,7 +5462,7 @@ qemu-kvm -net nic,model=? /dev/null
   ...
   
 
-  
+  
   32
   
 
@@ -5477,6 +5477,9 @@ qemu-kvm -net nic,model=? /dev/null
 The optional server element can be used to configure an
   ivshmem device connected to the ivshmem server via a unix socket. The
   path attribute specifies the path to the unix socket.
+  The start attribute specifies if the libvirt must start
+  the ivshmem-server. By default, libvirt expects that the ivshmem-server 
is
+  already running on the host.
 
 size
 The optional size element specifies the size of the 
shared memory.
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 64abf2b..a601747 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3246,6 +3246,9 @@

  

+ 
+   
+ 
  


diff --git a/po/POTFILES.in b/po/POTFILES.in
index f17b35f..7d90517 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -177,6 +177,7 @@ src/util/viridentity.c
 src/util/virinitctl.c
 src/util/viriptables.c
 src/util/viriscsi.c
+src/util/virivshmemserver.c
 src/util/virjson.c
 src/util/virkeyfile.c
 src/util/virlockspace.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 538530e..00e1ccf 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -118,6 +118,7 @@ UTIL_SOURCES =  
\
util/virinitctl.c util/virinitctl.h \
util/viriptables.c util/viriptables.h   \
util/viriscsi.c util/viriscsi.h \
+   util/virivshmemserver.c util/virivshmemserver.h \
util/virjson.c util/virjson.h   \
util/virkeycode.c util/virkeycode.h \
util/virkeyfile.c util/virkeyfile.h \
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 08d653a..224b367 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9509,6 +9509,7 @@ virDomainIvshmemDefParseXML(xmlNodePtr node,
 {
 char *ioeventfd = NULL;
 char *vectors = NULL;
+char *start = NULL;
 xmlNodePtr cur;
 xmlNodePtr save = ctxt->node;
 int ret;
@@ -9523,6 +9524,13 @@ virDomainIvshmemDefParseXML(xmlNodePtr node,
_("cannot parse