Re: [libvirt PATCH v2 4/9] qemu: add a DBus daemon helper unit

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 2/25/20 10:55 AM, marcandre.lureau@xxxxxxxxxx wrote:
> From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
>
> Add a unit to start & stop a private dbus-daemon.
>
> The daemon is meant to be started on demand, and associated with a
> QEMU process. It should be stopped when the QEMU process is stopped.
>
> The current policy is permissive like a session bus. Stricter
> policies can be added later, following recommendations from:
> https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/dbus.rst
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> ---
>   po/POTFILES.in           |   1 +
>   src/qemu/Makefile.inc.am |   2 +
>   src/qemu/qemu_dbus.c     | 282 +++++++++++++++++++++++++++++++++++++++
>   src/qemu/qemu_dbus.h     |  33 +++++
>   src/qemu/qemu_domain.h   |   2 +
>   5 files changed, 320 insertions(+)
>   create mode 100644 src/qemu/qemu_dbus.c
>   create mode 100644 src/qemu/qemu_dbus.h

> diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c
> new file mode 100644
> index 0000000000..383efa0209
> --- /dev/null
> +++ b/src/qemu/qemu_dbus.c
> @@ -0,0 +1,282 @@


> +void
> +qemuDBusStop(virQEMUDriverPtr driver,
> +             virDomainObjPtr vm)
> +{
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    g_autofree char *shortName = NULL;
> +    g_autofree char *pidfile = NULL;
> +    g_autofree char *configfile = NULL;
> +
> +    if (!(shortName = virDomainDefGetShortName(vm->def)))
> +        return;
> +
> +    pidfile = qemuDBusCreatePidFilename(cfg, shortName);
> +    configfile = qemuDBusCreateConfPath(cfg, shortName);
> +
> +    if (virPidFileForceCleanupPath(pidfile) < 0) {
> +        VIR_WARN("Unable to kill dbus-daemon process");
> +    } else {
> +        if (unlink(configfile) < 0 &&
> +            errno != ENOENT) {
> +            virReportSystemError(errno,
> +                                 _("Unable to remove stale configfile %s"),
> +                                 pidfile);
> +
> +        }

This unlink is needless as it's done by virPidFileForceCleanupPath(). 
Unfortunatelly, I might have directed you in a not so good way in my 
review of v1. I thought that virCommandSetPidFile() will cause the 
daemon to open and hold the pidfile opened. Which is not the case. 
Therefore, virPidFileForceCleanupPath() will see unlocked pidfile and 
assumes that the daemon had died meanwhle and left the pidfile behind. 
I've posted patches for that:

https://www.redhat.com/archives/libvir-list/2020-March/msg00499.html

Sorry. But once I merge them, this will start to work as expected.

> +        priv->dbusDaemonRunning = false;
> +    }
> +}
> +

Michal