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

[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