Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute
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