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 <jto...@redhat.com>:
>
> 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 <kshch...@redhat.com>
>> ---
>> 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>
