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

[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
>pidfile it first created ?
>

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

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140828/bde31d95/attachment-0001.sig>