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

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 :)