Re: [PATCH 1/3] ch: virCHMonitorNew() run new CH monitor daemonized

Thanks Jan, Didn’t noticed the issue with the white symbols when submitted a patch, next time will keep it in mind and be careful. Btw, do we have some tools to check and warn such possible issues?

Kirill > 2. 4. 2025 v 18:36, Ján Tomko <jtomko(a)redhat.com&gt;: > > On a Tuesday in 2025, Kirill Shchetiniuk via Devel wrote: >> When the new CH monitor was started, it ran as a non-daemonized >> process and was a child of the CH driver process. This led to a >> situation where if the CH driver died, the monitor process were >> killed too, terminating the running VM under the monitor. This >> led to termination of all VM started under the libvirt. >> >> Make new monitor running daemonized to avoid VMs shutdown when >> driver dies. Also added a pidfile its preparetion to be able >> to aquire daemon's PID. >> >> Signed-off-by: Kirill Shchetiniuk <kshcheti(a)redhat.com&gt; >> --- >> src/ch/ch_domain.c | 1 + >> src/ch/ch_domain.h | 1 + >> src/ch/ch_monitor.c | 24 ++++++++++++++++++++++-- >> src/ch/ch_process.c | 16 ++++++++++++++++ >> 4 files changed, 40 insertions(+), 2 deletions(-) >> >> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c >> index 0ba927a194..1dc085a755 100644 >> --- a/src/ch/ch_monitor.c >> +++ b/src/ch/ch_monitor.c >> @@ -644,6 +648,7 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg, int logfile) >> virCommandSetErrorFD(cmd, &logfile); >> virCommandNonblockingFDs(cmd); >> virCommandSetUmask(cmd, 0x002); >> + > > Nitpick: unrelated whitespace change. In some projects, these should be > squashed together with the patch changing the function as you've done here, > but in libvirt we separate them from the functional changes > (it's easier to review a patch that changes just whitespace, then the > actual functional changes are smaller) > >> socket_fd = chMonitorCreateSocket(mon->socketpath); >> if (socket_fd < 0) { >> virReportSystemError(errno, >> @@ -655,13 +660,28 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg, int logfile) >> virCommandAddArg(cmd, "--api-socket"); >> virCommandAddArgFormat(cmd, "fd=%d", socket_fd); >> virCommandPassFD(cmd, socket_fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); >> - > > This newline could have stayed - it seprated the arguments. > >> virCommandAddArg(cmd, "--event-monitor"); >> virCommandAddArgFormat(cmd, "path=%s", mon->eventmonitorpath); >> + virCommandSetPidFile(cmd, priv->pidfile); >> + virCommandDaemonize(cmd); >> >> /* launch Cloud-Hypervisor socket */ >> - if (virCommandRunAsync(cmd, &mon->pid) < 0) >> + rv = virCommandRun(cmd, NULL); >> + >> + if (rv == 0) { >> + if ((rv = virPidFileReadPath(priv->pidfile, &mon->pid)) < 0) { >> + virReportSystemError(-rv, >> + _("Domain %1$s didn't show up"), > > Not a nitpick - double spaces in the error message. This is > user-visible. > > Jano > >> + vm->def->name); >> + return NULL; >> + } >> + VIR_DEBUG("CH vm=%p name=%s running with pid=%lld", >> + vm, vm->def->name, (long long)vm->pid); >> + } else { >> + VIR_DEBUG("CH vm=%p name=%s failed to spawn", >> + vm, vm->def->name); >> return NULL; >> + } >> >> /* open the reader end of fifo before start Event Handler */ >> while ((event_monitor_fd = open(mon->eventmonitorpath, O_RDONLY)) < 0) { > <signature.asc>