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

[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