Re: [PATCH 14/19] qemu/dbus: log daemon stdout/err

On Wed, Jan 29, 2025 at 05:40:36PM +0400, marcandre.lureau(a)redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau(a)redhat.com&gt; > Currently, if dbus-daemon writes on errfd, it will SIGPIPE. > Signed-off-by: Marc-André Lureau <marcandre.lureau(a)redhat.com&gt; > --- > src/qemu/qemu_dbus.c | 61 +++++++++++++++++++++++++++++++++++--------- > 1 file changed, 49 insertions(+), 12 deletions(-) > diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c > index 4a019ae092..0e22392e6f 100644 > --- a/src/qemu/qemu_dbus.c > +++ b/src/qemu/qemu_dbus.c > @@ -18,12 +18,17 @@ > > #include <config.h> > > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <fcntl.h> > + > #include "qemu_dbus.h" > #include "qemu_security.h" > > #include "virlog.h" > #include "virtime.h" > #include "virpidfile.h" > +#include "virutil.h" > > #define VIR_FROM_THIS VIR_FROM_NONE > > @@ -67,6 +72,14 @@ qemuDBusCreateConfPath(virQEMUDriverConfig *cfg, > } > > > +static char * > +qemuDBusCreateLogFilename(virQEMUDriverConfig *cfg, > + const char *shortName) > +{ > + return qemuDBusCreateFilename(cfg->dbusStateDir, shortName, ".log"); > +} > + > + > char * > qemuDBusGetAddress(virQEMUDriver *driver, > virDomainObj *vm) > @@ -183,10 +196,11 @@ qemuDBusStart(virQEMUDriver *driver, > g_autofree char *pidfile = NULL; > g_autofree char *configfile = NULL; > g_autofree char *sockpath = NULL; > + g_autofree char *logpath = NULL; > g_autofree char *address = NULL; > virTimeBackOffVar timebackoff; > const unsigned long long timeout = 500 * 1000; /* ms */ > - VIR_AUTOCLOSE errfd = -1; > + VIR_AUTOCLOSE logfd = -1; > pid_t cpid = -1; > int ret = -1; > > @@ -219,10 +233,40 @@ qemuDBusStart(virQEMUDriver *driver, > if (qemuSecurityDomainSetPathLabel(driver, vm, configfile, false) < 0) > goto cleanup; > > + logpath = qemuDBusCreateLogFilename(cfg, shortName); > + > + if (cfg->stdioLogD) { > + g_autoptr(virLogManager) logManager = virLogManagerNew(driver->privileged); > + > + if (!logManager) > + goto cleanup; > + > + if ((logfd = virLogManagerDomainOpenLogFile(logManager, > + "qemu", > + vm->def->uuid, > + vm->def->name, > + logpath, > + 0, > + NULL, NULL)) < 0) > + goto cleanup; Hmm, this is going to make the dbus logs be intermingled with the QEMU logs which feels undesirable to me. IIUC for swtpm we have a dedicated log file per VM, and probably ought to follow that pattern generally for any helper app we spawn off.

> @@ -239,22 +283,15 @@ qemuDBusStart(virQEMUDriver *driver, > if (virTimeBackOffStart(&timebackoff, 1, timeout) < 0) > goto cleanup; > while (virTimeBackOffWait(&timebackoff)) { > - char errbuf[1024] = { 0 }; > - > if (virFileExists(sockpath)) > break; > > if (virProcessKill(cpid, 0) == 0) > continue; > > - if (saferead(errfd, errbuf, sizeof(errbuf) - 1) < 0) { > - virReportSystemError(errno, > - _("dbus-daemon %1$s died unexpectedly"), > - dbusDaemonPath); > - } else { > - virReportError(VIR_ERR_OPERATION_FAILED, > - _("dbus-daemon died and reported: %1$s"), errbuf); > - } > + virReportSystemError(errno, > + _("dbus-daemon %1$s died unexpectedly, check the log"), > + dbusDaemonPath);