Use a FIFO(named pipe) for --event-monitor option in CH. Introduce a new
thread, `virCHEventHandlerLoop`, to continuously monitor and handle
events from cloud-hypervisor.
Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@linux.microsoft.com>
Co-authored-by: Vineeth Pillai <viremana@linux.microsoft.com>
---
po/POTFILES | 1 +
src/ch/ch_events.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
src/ch/ch_events.h | 26 ++++++++++++
src/ch/ch_monitor.c | 28 +++++++++++++
src/ch/ch_monitor.h | 3 ++
src/ch/meson.build | 2 +
6 files changed, 156 insertions(+)
create mode 100644 src/ch/ch_events.c
create mode 100644 src/ch/ch_events.h
diff --git a/po/POTFILES b/po/POTFILES
index 1ed4086d2c..d53307cec4 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -21,6 +21,7 @@ src/bhyve/bhyve_process.c
src/ch/ch_conf.c
src/ch/ch_domain.c
src/ch/ch_driver.c
+src/ch/ch_events.c
src/ch/ch_interface.c
src/ch/ch_monitor.c
src/ch/ch_process.c
diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c
new file mode 100644
index 0000000000..851fbc9f26
--- /dev/null
+++ b/src/ch/ch_events.c
@@ -0,0 +1,96 @@
+/*
+ * Copyright Microsoft Corp. 2024
+ *
+ * ch_events.h: Handle Cloud-Hypervisor events
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#include <config.h>
+
+#include <fcntl.h>
+
+#include "ch_events.h"
+#include "virfile.h"
+#include "virlog.h"
+
+VIR_LOG_INIT("ch.ch_events");
+
+static void virCHEventHandlerLoop(void *data)
+{
+ virCHMonitor *mon = data;
+ virDomainObj *vm = NULL;
+ int event_monitor_fd;
+
+ VIR_DEBUG("Event handler loop thread starting");
+
+ while ((event_monitor_fd = open(mon->eventmonitorpath, O_RDONLY)) < 0) {
+ if (errno == EINTR) {
+ g_usleep(100000); // 100 milli seconds
+ continue;
+ }
+ /*
+ * Any other error should be a BUG(kernel/libc/libvirtd)
+ * (ENOMEM can happen on exceeding per-user limits)
+ */
+ VIR_ERROR(_("Failed to open the event monitor FIFO(%1$s) read end!"),
+ mon->eventmonitorpath);
+ abort();
+ }
+ VIR_DEBUG("Opened the event monitor FIFO(%s)", mon->eventmonitorpath);
+
+ /*
+ * We would need to wait until VM is initialized.
+ */
+ while (!(vm = virObjectRef(mon->vm)))
+ g_usleep(100000); // 100 milli seconds
+
+ while (g_atomic_int_get(&mon->event_handler_stop) == 0) {
+ VIR_DEBUG("Reading events from event monitor file...");
+ /* Read and process events here */
+ }
+
+ VIR_FORCE_CLOSE(event_monitor_fd);
+ virObjectUnref(vm);
+
+ VIR_DEBUG("Event handler loop thread exiting");
+ return;
+}
+
+int virCHStartEventHandler(virCHMonitor *mon)
+{
+ g_autofree char *name = NULL;
+ name = g_strdup_printf("event-handler-%d", mon->pid);
+
+ virObjectRef(mon);
+ if (virThreadCreateFull(&mon->event_handler_thread,
+ false,
+ virCHEventHandlerLoop,
+ name,
+ false,
+ mon) < 0) {
+ virObjectUnref(mon);
+ return -1;
+ }
+ virObjectUnref(mon);
+
+ g_atomic_int_set(&mon->event_handler_stop, 0);
+ return 0;
+}
+
+void virCHStopEventHandler(virCHMonitor *mon)
+{
+ g_atomic_int_set(&mon->event_handler_stop, 1);
+}
diff --git a/src/ch/ch_events.h b/src/ch/ch_events.h
new file mode 100644
index 0000000000..4c8a48231d
--- /dev/null
+++ b/src/ch/ch_events.h
@@ -0,0 +1,26 @@
+/*
+ * Copyright Microsoft Corp. 2024
+ *
+ * ch_events.h: header file for handling Cloud-Hypervisor events
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include "ch_monitor.h"
+
+int virCHStartEventHandler(virCHMonitor *mon);
+void virCHStopEventHandler(virCHMonitor *mon);
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index 8c99fe1019..5e43bf9ef8 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -26,6 +26,7 @@
#include "datatypes.h"
#include "ch_conf.h"
+#include "ch_events.h"
#include "ch_interface.h"
#include "ch_monitor.h"
#include "domain_interface.h"
@@ -572,6 +573,28 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
return NULL;
}
+ /* Event monitor file to listen for VM state changes */
+ mon->eventmonitorpath = g_strdup_printf("%s/%s-event-monitor-fifo",
+ cfg->stateDir, vm->def->name);
+ if (virFileExists(mon->eventmonitorpath) &&
+ !virFileIsNamedPipe(mon->eventmonitorpath)) {
+ VIR_WARN("Monitor file (%s) is not a FIFO, trying to delete!",
+ mon->eventmonitorpath);
+ if (virFileRemove(mon->eventmonitorpath, -1, -1) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to remove the file: %1$s"),
+ mon->eventmonitorpath);
+ return NULL;
+ }
+ }
+
+ if (mkfifo(mon->eventmonitorpath, S_IWUSR | S_IRUSR) < 0 &&
+ errno != EEXIST) {
+ virReportSystemError(errno, "%s",
+ _("Cannot create monitor FIFO"));
+ return NULL;
+ }
+
cmd = virCommandNew(vm->def->emulator);
virCommandSetUmask(cmd, 0x002);
socket_fd = chMonitorCreateSocket(mon->socketpath);
@@ -593,6 +616,9 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
if (virCommandRunAsync(cmd, &mon->pid) < 0)
return NULL;
+ if (virCHStartEventHandler(mon) < 0)
+ return NULL;
+
/* get a curl handle */
mon->handle = curl_easy_init();
@@ -641,6 +667,8 @@ void virCHMonitorClose(virCHMonitor *mon)
g_free(mon->eventmonitorpath);
}
+ virCHStopEventHandler(mon);
+
virObjectUnref(mon);
}
diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h
index 2ef8706b99..878a185f29 100644
--- a/src/ch/ch_monitor.h
+++ b/src/ch/ch_monitor.h
@@ -97,6 +97,9 @@ struct _virCHMonitor {
char *eventmonitorpath;
+ virThread event_handler_thread;
+ int event_handler_stop;
+
pid_t pid;
virDomainObj *vm;
diff --git a/src/ch/meson.build b/src/ch/meson.build
index 633966aac7..684beac1f2 100644
--- a/src/ch/meson.build
+++ b/src/ch/meson.build
@@ -7,6 +7,8 @@ ch_driver_sources = [
'ch_domain.h',
'ch_driver.c',
'ch_driver.h',
+ 'ch_events.c',
+ 'ch_driver.h',
'ch_interface.c',
'ch_interface.h',
'ch_monitor.c',
--
2.34.1
On 9/19/2024 8:02 AM, Purna Pavan Chandra Aekkaladevi wrote:
> Use a FIFO(named pipe) for --event-monitor option in CH. Introduce a new
> thread, `virCHEventHandlerLoop`, to continuously monitor and handle
> events from cloud-hypervisor.
>
> Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@linux.microsoft.com>
> Co-authored-by: Vineeth Pillai <viremana@linux.microsoft.com>
> ---
> po/POTFILES | 1 +
> src/ch/ch_events.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
> src/ch/ch_events.h | 26 ++++++++++++
> src/ch/ch_monitor.c | 28 +++++++++++++
> src/ch/ch_monitor.h | 3 ++
> src/ch/meson.build | 2 +
> 6 files changed, 156 insertions(+)
> create mode 100644 src/ch/ch_events.c
> create mode 100644 src/ch/ch_events.h
>
> diff --git a/po/POTFILES b/po/POTFILES
> index 1ed4086d2c..d53307cec4 100644
> --- a/po/POTFILES
> +++ b/po/POTFILES
> @@ -21,6 +21,7 @@ src/bhyve/bhyve_process.c
> src/ch/ch_conf.c
> src/ch/ch_domain.c
> src/ch/ch_driver.c
> +src/ch/ch_events.c
> src/ch/ch_interface.c
> src/ch/ch_monitor.c
> src/ch/ch_process.c
> diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c
> new file mode 100644
> index 0000000000..851fbc9f26
> --- /dev/null
> +++ b/src/ch/ch_events.c
> @@ -0,0 +1,96 @@
> +/*
> + * Copyright Microsoft Corp. 2024
> + *
> + * ch_events.h: Handle Cloud-Hypervisor events
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <config.h>
> +
> +#include <fcntl.h>
> +
> +#include "ch_events.h"
> +#include "virfile.h"
> +#include "virlog.h"
> +
> +VIR_LOG_INIT("ch.ch_events");
> +
> +static void virCHEventHandlerLoop(void *data)
> +{
> + virCHMonitor *mon = data;
> + virDomainObj *vm = NULL;
> + int event_monitor_fd;
> +
> + VIR_DEBUG("Event handler loop thread starting");
> +
> + while ((event_monitor_fd = open(mon->eventmonitorpath, O_RDONLY)) < 0) {
> + if (errno == EINTR) {
> + g_usleep(100000); // 100 milli seconds
> + continue;
> + }
> + /*
> + * Any other error should be a BUG(kernel/libc/libvirtd)
> + * (ENOMEM can happen on exceeding per-user limits)
> + */
> + VIR_ERROR(_("Failed to open the event monitor FIFO(%1$s) read end!"),
> + mon->eventmonitorpath);
> + abort();
> + }
> + VIR_DEBUG("Opened the event monitor FIFO(%s)", mon->eventmonitorpath);
> +
> + /*
> + * We would need to wait until VM is initialized.
> + */
> + while (!(vm = virObjectRef(mon->vm)))
> + g_usleep(100000); // 100 milli seconds
> +
> + while (g_atomic_int_get(&mon->event_handler_stop) == 0) {
> + VIR_DEBUG("Reading events from event monitor file...");
> + /* Read and process events here */
> + }
> +
> + VIR_FORCE_CLOSE(event_monitor_fd);
> + virObjectUnref(vm);
> +
> + VIR_DEBUG("Event handler loop thread exiting");
> + return;
> +}
> +
> +int virCHStartEventHandler(virCHMonitor *mon)
> +{
> + g_autofree char *name = NULL;
> + name = g_strdup_printf("event-handler-%d", mon->pid);
> +
> + virObjectRef(mon);
> + if (virThreadCreateFull(&mon->event_handler_thread,
> + false,
> + virCHEventHandlerLoop,
> + name,
> + false,
> + mon) < 0) {
> + virObjectUnref(mon);
> + return -1;
> + }
> + virObjectUnref(mon);
> +
> + g_atomic_int_set(&mon->event_handler_stop, 0);
> + return 0;
> +}
> +
> +void virCHStopEventHandler(virCHMonitor *mon)
> +{
> + g_atomic_int_set(&mon->event_handler_stop, 1);
> +}
> diff --git a/src/ch/ch_events.h b/src/ch/ch_events.h
> new file mode 100644
> index 0000000000..4c8a48231d
> --- /dev/null
> +++ b/src/ch/ch_events.h
> @@ -0,0 +1,26 @@
> +/*
> + * Copyright Microsoft Corp. 2024
> + *
> + * ch_events.h: header file for handling Cloud-Hypervisor events
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#pragma once
> +
> +#include "ch_monitor.h"
> +
> +int virCHStartEventHandler(virCHMonitor *mon);
> +void virCHStopEventHandler(virCHMonitor *mon);
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index 8c99fe1019..5e43bf9ef8 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -26,6 +26,7 @@
>
> #include "datatypes.h"
> #include "ch_conf.h"
> +#include "ch_events.h"
> #include "ch_interface.h"
> #include "ch_monitor.h"
> #include "domain_interface.h"
> @@ -572,6 +573,28 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
> return NULL;
> }
>
> + /* Event monitor file to listen for VM state changes */
> + mon->eventmonitorpath = g_strdup_printf("%s/%s-event-monitor-fifo",
> + cfg->stateDir, vm->def->name);
> + if (virFileExists(mon->eventmonitorpath) &&
> + !virFileIsNamedPipe(mon->eventmonitorpath)) {
> + VIR_WARN("Monitor file (%s) is not a FIFO, trying to delete!",
> + mon->eventmonitorpath);
> + if (virFileRemove(mon->eventmonitorpath, -1, -1) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to remove the file: %1$s"),
> + mon->eventmonitorpath);
> + return NULL;
> + }
> + }
> +
> + if (mkfifo(mon->eventmonitorpath, S_IWUSR | S_IRUSR) < 0 &&
> + errno != EEXIST) {
> + virReportSystemError(errno, "%s",
> + _("Cannot create monitor FIFO"));
> + return NULL;
> + }
Why do you need to create a pipe here as instead of opening the file at
eventmonitorpath, after cloud-hypervisor starts?
> +
> cmd = virCommandNew(vm->def->emulator);
> virCommandSetUmask(cmd, 0x002);
> socket_fd = chMonitorCreateSocket(mon->socketpath);
> @@ -593,6 +616,9 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
> if (virCommandRunAsync(cmd, &mon->pid) < 0)
> return NULL;
>
> + if (virCHStartEventHandler(mon) < 0)
> + return NULL;
> +
> /* get a curl handle */
> mon->handle = curl_easy_init();
>
> @@ -641,6 +667,8 @@ void virCHMonitorClose(virCHMonitor *mon)
> g_free(mon->eventmonitorpath);
> }
>
> + virCHStopEventHandler(mon);
> +
> virObjectUnref(mon);
> }
>
> diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h
> index 2ef8706b99..878a185f29 100644
> --- a/src/ch/ch_monitor.h
> +++ b/src/ch/ch_monitor.h
> @@ -97,6 +97,9 @@ struct _virCHMonitor {
>
> char *eventmonitorpath;
>
> + virThread event_handler_thread;
> + int event_handler_stop;
> +
> pid_t pid;
>
> virDomainObj *vm;
> diff --git a/src/ch/meson.build b/src/ch/meson.build
> index 633966aac7..684beac1f2 100644
> --- a/src/ch/meson.build
> +++ b/src/ch/meson.build
> @@ -7,6 +7,8 @@ ch_driver_sources = [
> 'ch_domain.h',
> 'ch_driver.c',
> 'ch_driver.h',
> + 'ch_events.c',
> + 'ch_driver.h',
> 'ch_interface.c',
> 'ch_interface.h',
> 'ch_monitor.c',
--
Regards,
Praveen K Paladugu
On Thu, Sep 26, 2024 at 11:00:26AM -0500, Praveen K Paladugu wrote:
>
>
> On 9/19/2024 8:02 AM, Purna Pavan Chandra Aekkaladevi wrote:
> > Use a FIFO(named pipe) for --event-monitor option in CH. Introduce a new
> > thread, `virCHEventHandlerLoop`, to continuously monitor and handle
> > events from cloud-hypervisor.
> >
> > Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@linux.microsoft.com>
> > Co-authored-by: Vineeth Pillai <viremana@linux.microsoft.com>
> > ---
> > po/POTFILES | 1 +
> > src/ch/ch_events.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
> > src/ch/ch_events.h | 26 ++++++++++++
> > src/ch/ch_monitor.c | 28 +++++++++++++
> > src/ch/ch_monitor.h | 3 ++
> > src/ch/meson.build | 2 +
> > 6 files changed, 156 insertions(+)
> > create mode 100644 src/ch/ch_events.c
> > create mode 100644 src/ch/ch_events.h
> >
> > diff --git a/po/POTFILES b/po/POTFILES
> > index 1ed4086d2c..d53307cec4 100644
> > --- a/po/POTFILES
> > +++ b/po/POTFILES
> > @@ -21,6 +21,7 @@ src/bhyve/bhyve_process.c
> > src/ch/ch_conf.c
> > src/ch/ch_domain.c
> > src/ch/ch_driver.c
> > +src/ch/ch_events.c
> > src/ch/ch_interface.c
> > src/ch/ch_monitor.c
> > src/ch/ch_process.c
> > diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c
> > new file mode 100644
> > index 0000000000..851fbc9f26
> > --- /dev/null
> > +++ b/src/ch/ch_events.c
> > @@ -0,0 +1,96 @@
> > +/*
> > + * Copyright Microsoft Corp. 2024
> > + *
> > + * ch_events.h: Handle Cloud-Hypervisor events
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library. If not, see
> > + * <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include <fcntl.h>
> > +
> > +#include "ch_events.h"
> > +#include "virfile.h"
> > +#include "virlog.h"
> > +
> > +VIR_LOG_INIT("ch.ch_events");
> > +
> > +static void virCHEventHandlerLoop(void *data)
> > +{
> > + virCHMonitor *mon = data;
> > + virDomainObj *vm = NULL;
> > + int event_monitor_fd;
> > +
> > + VIR_DEBUG("Event handler loop thread starting");
> > +
> > + while ((event_monitor_fd = open(mon->eventmonitorpath, O_RDONLY)) < 0) {
> > + if (errno == EINTR) {
> > + g_usleep(100000); // 100 milli seconds
> > + continue;
> > + }
> > + /*
> > + * Any other error should be a BUG(kernel/libc/libvirtd)
> > + * (ENOMEM can happen on exceeding per-user limits)
> > + */
> > + VIR_ERROR(_("Failed to open the event monitor FIFO(%1$s) read end!"),
> > + mon->eventmonitorpath);
> > + abort();
> > + }
> > + VIR_DEBUG("Opened the event monitor FIFO(%s)", mon->eventmonitorpath);
> > +
> > + /*
> > + * We would need to wait until VM is initialized.
> > + */
> > + while (!(vm = virObjectRef(mon->vm)))
> > + g_usleep(100000); // 100 milli seconds
> > +
> > + while (g_atomic_int_get(&mon->event_handler_stop) == 0) {
> > + VIR_DEBUG("Reading events from event monitor file...");
> > + /* Read and process events here */
> > + }
> > +
> > + VIR_FORCE_CLOSE(event_monitor_fd);
> > + virObjectUnref(vm);
> > +
> > + VIR_DEBUG("Event handler loop thread exiting");
> > + return;
> > +}
> > +
> > +int virCHStartEventHandler(virCHMonitor *mon)
> > +{
> > + g_autofree char *name = NULL;
> > + name = g_strdup_printf("event-handler-%d", mon->pid);
> > +
> > + virObjectRef(mon);
> > + if (virThreadCreateFull(&mon->event_handler_thread,
> > + false,
> > + virCHEventHandlerLoop,
> > + name,
> > + false,
> > + mon) < 0) {
> > + virObjectUnref(mon);
> > + return -1;
> > + }
> > + virObjectUnref(mon);
> > +
> > + g_atomic_int_set(&mon->event_handler_stop, 0);
> > + return 0;
> > +}
> > +
> > +void virCHStopEventHandler(virCHMonitor *mon)
> > +{
> > + g_atomic_int_set(&mon->event_handler_stop, 1);
> > +}
> > diff --git a/src/ch/ch_events.h b/src/ch/ch_events.h
> > new file mode 100644
> > index 0000000000..4c8a48231d
> > --- /dev/null
> > +++ b/src/ch/ch_events.h
> > @@ -0,0 +1,26 @@
> > +/*
> > + * Copyright Microsoft Corp. 2024
> > + *
> > + * ch_events.h: header file for handling Cloud-Hypervisor events
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library. If not, see
> > + * <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#pragma once
> > +
> > +#include "ch_monitor.h"
> > +
> > +int virCHStartEventHandler(virCHMonitor *mon);
> > +void virCHStopEventHandler(virCHMonitor *mon);
> > diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> > index 8c99fe1019..5e43bf9ef8 100644
> > --- a/src/ch/ch_monitor.c
> > +++ b/src/ch/ch_monitor.c
> > @@ -26,6 +26,7 @@
> > #include "datatypes.h"
> > #include "ch_conf.h"
> > +#include "ch_events.h"
> > #include "ch_interface.h"
> > #include "ch_monitor.h"
> > #include "domain_interface.h"
> > @@ -572,6 +573,28 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
> > return NULL;
> > }
> > + /* Event monitor file to listen for VM state changes */
> > + mon->eventmonitorpath = g_strdup_printf("%s/%s-event-monitor-fifo",
> > + cfg->stateDir, vm->def->name);
> > + if (virFileExists(mon->eventmonitorpath) &&
> > + !virFileIsNamedPipe(mon->eventmonitorpath)) {
> > + VIR_WARN("Monitor file (%s) is not a FIFO, trying to delete!",
> > + mon->eventmonitorpath);
> > + if (virFileRemove(mon->eventmonitorpath, -1, -1) < 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Failed to remove the file: %1$s"),
> > + mon->eventmonitorpath);
> > + return NULL;
> > + }
> > + }
> > +
> > + if (mkfifo(mon->eventmonitorpath, S_IWUSR | S_IRUSR) < 0 &&
> > + errno != EEXIST) {
> > + virReportSystemError(errno, "%s",
> > + _("Cannot create monitor FIFO"));
> > + return NULL;
> > + }
>
> Why do you need to create a pipe here as instead of opening the file at
> eventmonitorpath, after cloud-hypervisor starts?
>
Since the scenario here is of reader and writer form, named pipe(fifo) suits best.
Also, fifo is faster as it doesn't involve writing into the actual
storage.
"
When processes are exchanging data via the FIFO, the kernel passes all data
internally without writing it to the filesystem.
"
Ref: https://www.man7.org/linux/man-pages/man7/fifo.7.html
> > +
> > cmd = virCommandNew(vm->def->emulator);
> > virCommandSetUmask(cmd, 0x002);
> > socket_fd = chMonitorCreateSocket(mon->socketpath);
> > @@ -593,6 +616,9 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
> > if (virCommandRunAsync(cmd, &mon->pid) < 0)
> > return NULL;
> > + if (virCHStartEventHandler(mon) < 0)
> > + return NULL;
> > +
> > /* get a curl handle */
> > mon->handle = curl_easy_init();
> > @@ -641,6 +667,8 @@ void virCHMonitorClose(virCHMonitor *mon)
> > g_free(mon->eventmonitorpath);
> > }
> > + virCHStopEventHandler(mon);
> > +
> > virObjectUnref(mon);
> > }
> > diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h
> > index 2ef8706b99..878a185f29 100644
> > --- a/src/ch/ch_monitor.h
> > +++ b/src/ch/ch_monitor.h
> > @@ -97,6 +97,9 @@ struct _virCHMonitor {
> > char *eventmonitorpath;
> > + virThread event_handler_thread;
> > + int event_handler_stop;
> > +
> > pid_t pid;
> > virDomainObj *vm;
> > diff --git a/src/ch/meson.build b/src/ch/meson.build
> > index 633966aac7..684beac1f2 100644
> > --- a/src/ch/meson.build
> > +++ b/src/ch/meson.build
> > @@ -7,6 +7,8 @@ ch_driver_sources = [
> > 'ch_domain.h',
> > 'ch_driver.c',
> > 'ch_driver.h',
> > + 'ch_events.c',
> > + 'ch_driver.h',
> > 'ch_interface.c',
> > 'ch_interface.h',
> > 'ch_monitor.c',
>
> --
> Regards,
> Praveen K Paladugu
Regards,
Purna Pavan Chandra
On 9/19/2024 8:02 AM, Purna Pavan Chandra Aekkaladevi wrote:
> Use a FIFO(named pipe) for --event-monitor option in CH. Introduce a new
> thread, `virCHEventHandlerLoop`, to continuously monitor and handle
> events from cloud-hypervisor.
>
> Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@linux.microsoft.com>
> Co-authored-by: Vineeth Pillai <viremana@linux.microsoft.com>
> ---
> po/POTFILES | 1 +
> src/ch/ch_events.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
> src/ch/ch_events.h | 26 ++++++++++++
> src/ch/ch_monitor.c | 28 +++++++++++++
> src/ch/ch_monitor.h | 3 ++
> src/ch/meson.build | 2 +
> 6 files changed, 156 insertions(+)
> create mode 100644 src/ch/ch_events.c
> create mode 100644 src/ch/ch_events.h
>
> diff --git a/po/POTFILES b/po/POTFILES
> index 1ed4086d2c..d53307cec4 100644
> --- a/po/POTFILES
> +++ b/po/POTFILES
> @@ -21,6 +21,7 @@ src/bhyve/bhyve_process.c
> src/ch/ch_conf.c
> src/ch/ch_domain.c
> src/ch/ch_driver.c
> +src/ch/ch_events.c
> src/ch/ch_interface.c
> src/ch/ch_monitor.c
> src/ch/ch_process.c
> diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c
> new file mode 100644
> index 0000000000..851fbc9f26
> --- /dev/null
> +++ b/src/ch/ch_events.c
> @@ -0,0 +1,96 @@
> +/*
> + * Copyright Microsoft Corp. 2024
> + *
> + * ch_events.h: Handle Cloud-Hypervisor events
please fix the filename here.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <config.h>
> +
> +#include <fcntl.h>
> +
> +#include "ch_events.h"
> +#include "virfile.h"
> +#include "virlog.h"
> +
> +VIR_LOG_INIT("ch.ch_events");
> +
> +static void virCHEventHandlerLoop(void *data)
> +{
> + virCHMonitor *mon = data;
> + virDomainObj *vm = NULL;
> + int event_monitor_fd;
> +
> + VIR_DEBUG("Event handler loop thread starting");
> +
> + while ((event_monitor_fd = open(mon->eventmonitorpath, O_RDONLY)) < 0) {
> + if (errno == EINTR) {
> + g_usleep(100000); // 100 milli seconds
> + continue;
> + }
> + /*
> + * Any other error should be a BUG(kernel/libc/libvirtd)
> + * (ENOMEM can happen on exceeding per-user limits)
> + */
> + VIR_ERROR(_("Failed to open the event monitor FIFO(%1$s) read end!"),
> + mon->eventmonitorpath);
> + abort();
> + }
> + VIR_DEBUG("Opened the event monitor FIFO(%s)", mon->eventmonitorpath);
> +
> + /*
> + * We would need to wait until VM is initialized.
> + */
Does this comment apply to next while loop where the events are
processed?
> + while (!(vm = virObjectRef(mon->vm)))
> + g_usleep(100000); // 100 milli seconds
In this while loop you are only getting a reference to vm object.
> +
> + while (g_atomic_int_get(&mon->event_handler_stop) == 0) {
> + VIR_DEBUG("Reading events from event monitor file...");
> + /* Read and process events here */
> + }
> +
> + VIR_FORCE_CLOSE(event_monitor_fd);
> + virObjectUnref(vm);
> +
> + VIR_DEBUG("Event handler loop thread exiting");
> + return;
> +}
> +
> +int virCHStartEventHandler(virCHMonitor *mon)
> +{
> + g_autofree char *name = NULL;
> + name = g_strdup_printf("event-handler-%d", mon->pid);
It would be better to name the thread something similar to
"event-handler_ch_${pid}_${vmname}". A similar format is
used for cgroup naming.
> +
> + virObjectRef(mon);
> + if (virThreadCreateFull(&mon->event_handler_thread,
> + false,
> + virCHEventHandlerLoop,
> + name,
> + false,
> + mon) < 0) {
> + virObjectUnref(mon);
> + return -1;
> + }
> + virObjectUnref(mon);
> +
> + g_atomic_int_set(&mon->event_handler_stop, 0);
> + return 0;
> +}
> +
> +void virCHStopEventHandler(virCHMonitor *mon)
> +{
> + g_atomic_int_set(&mon->event_handler_stop, 1);
> +}
> diff --git a/src/ch/ch_events.h b/src/ch/ch_events.h
> new file mode 100644
> index 0000000000..4c8a48231d
> --- /dev/null
> +++ b/src/ch/ch_events.h
> @@ -0,0 +1,26 @@
> +/*
> + * Copyright Microsoft Corp. 2024
> + *
> + * ch_events.h: header file for handling Cloud-Hypervisor events
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#pragma once
> +
> +#include "ch_monitor.h"
> +
> +int virCHStartEventHandler(virCHMonitor *mon);
> +void virCHStopEventHandler(virCHMonitor *mon);
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index 8c99fe1019..5e43bf9ef8 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -26,6 +26,7 @@
>
> #include "datatypes.h"
> #include "ch_conf.h"
> +#include "ch_events.h"
> #include "ch_interface.h"
> #include "ch_monitor.h"
> #include "domain_interface.h"
> @@ -572,6 +573,28 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
> return NULL;
> }
>
> + /* Event monitor file to listen for VM state changes */
> + mon->eventmonitorpath = g_strdup_printf("%s/%s-event-monitor-fifo",
> + cfg->stateDir, vm->def->name);
> + if (virFileExists(mon->eventmonitorpath) &&
> + !virFileIsNamedPipe(mon->eventmonitorpath)) {
> + VIR_WARN("Monitor file (%s) is not a FIFO, trying to delete!",
> + mon->eventmonitorpath);
> + if (virFileRemove(mon->eventmonitorpath, -1, -1) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to remove the file: %1$s"),
> + mon->eventmonitorpath);
> + return NULL;
> + }
> + }
> +
This does not cover the case when eventmonitorpath exists, and is a
named pipe, may be, not cleaned up from a previous run.
You are better off just deleting the file and open a new one with
mkfifo below.
> + if (mkfifo(mon->eventmonitorpath, S_IWUSR | S_IRUSR) < 0 &&
> + errno != EEXIST) {
> + virReportSystemError(errno, "%s",
> + _("Cannot create monitor FIFO"));
> + return NULL;
> + }
> +
> cmd = virCommandNew(vm->def->emulator);
> virCommandSetUmask(cmd, 0x002);
> socket_fd = chMonitorCreateSocket(mon->socketpath);
> @@ -593,6 +616,9 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
> if (virCommandRunAsync(cmd, &mon->pid) < 0)
> return NULL;
>
> + if (virCHStartEventHandler(mon) < 0)
> + return NULL;
> +
> /* get a curl handle */
> mon->handle = curl_easy_init();
>
> @@ -641,6 +667,8 @@ void virCHMonitorClose(virCHMonitor *mon)
> g_free(mon->eventmonitorpath);
> }
>
> + virCHStopEventHandler(mon);
> +
Please move virCHStopEventHandler above `if (mon->eventmonitorpath) {`
section. Doing so, will allow the event monitor thread to cleanly exit,
before you invoke `virFileRemove(mon->eventmonitorpath...`
> virObjectUnref(mon);
> }
>
> diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h
> index 2ef8706b99..878a185f29 100644
> --- a/src/ch/ch_monitor.h
> +++ b/src/ch/ch_monitor.h
> @@ -97,6 +97,9 @@ struct _virCHMonitor {
>
> char *eventmonitorpath;
>
> + virThread event_handler_thread;
> + int event_handler_stop;
> +
> pid_t pid;
>
> virDomainObj *vm;
> diff --git a/src/ch/meson.build b/src/ch/meson.build
> index 633966aac7..684beac1f2 100644
> --- a/src/ch/meson.build
> +++ b/src/ch/meson.build
> @@ -7,6 +7,8 @@ ch_driver_sources = [
> 'ch_domain.h',
> 'ch_driver.c',
> 'ch_driver.h',
> + 'ch_events.c',
> + 'ch_driver.h',
> 'ch_interface.c',
> 'ch_interface.h',
> 'ch_monitor.c',
--
Regards,
Praveen K Paladugu
On Mon, Sep 23, 2024 at 04:29:35PM -0500, Praveen K Paladugu wrote:
>
>
> On 9/19/2024 8:02 AM, Purna Pavan Chandra Aekkaladevi wrote:
> >Use a FIFO(named pipe) for --event-monitor option in CH. Introduce a new
> >thread, `virCHEventHandlerLoop`, to continuously monitor and handle
> >events from cloud-hypervisor.
> >
> >Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@linux.microsoft.com>
> >Co-authored-by: Vineeth Pillai <viremana@linux.microsoft.com>
> >---
> > po/POTFILES | 1 +
> > src/ch/ch_events.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
> > src/ch/ch_events.h | 26 ++++++++++++
> > src/ch/ch_monitor.c | 28 +++++++++++++
> > src/ch/ch_monitor.h | 3 ++
> > src/ch/meson.build | 2 +
> > 6 files changed, 156 insertions(+)
> > create mode 100644 src/ch/ch_events.c
> > create mode 100644 src/ch/ch_events.h
> >
> >diff --git a/po/POTFILES b/po/POTFILES
> >index 1ed4086d2c..d53307cec4 100644
> >--- a/po/POTFILES
> >+++ b/po/POTFILES
> >@@ -21,6 +21,7 @@ src/bhyve/bhyve_process.c
> > src/ch/ch_conf.c
> > src/ch/ch_domain.c
> > src/ch/ch_driver.c
> >+src/ch/ch_events.c
> > src/ch/ch_interface.c
> > src/ch/ch_monitor.c
> > src/ch/ch_process.c
> >diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c
> >new file mode 100644
> >index 0000000000..851fbc9f26
> >--- /dev/null
> >+++ b/src/ch/ch_events.c
> >@@ -0,0 +1,96 @@
> >+/*
> >+ * Copyright Microsoft Corp. 2024
> >+ *
> >+ * ch_events.h: Handle Cloud-Hypervisor events
>
> please fix the filename here.
>
> >+ *
> >+ * This library is free software; you can redistribute it and/or
> >+ * modify it under the terms of the GNU Lesser General Public
> >+ * License as published by the Free Software Foundation; either
> >+ * version 2.1 of the License, or (at your option) any later version.
> >+ *
> >+ * This library is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> >+ * Lesser General Public License for more details.
> >+ *
> >+ * You should have received a copy of the GNU Lesser General Public
> >+ * License along with this library. If not, see
> >+ * <http://www.gnu.org/licenses/>.
> >+ */
> >+
> >+#include <config.h>
> >+
> >+#include <fcntl.h>
> >+
> >+#include "ch_events.h"
> >+#include "virfile.h"
> >+#include "virlog.h"
> >+
> >+VIR_LOG_INIT("ch.ch_events");
> >+
> >+static void virCHEventHandlerLoop(void *data)
> >+{
> >+ virCHMonitor *mon = data;
> >+ virDomainObj *vm = NULL;
> >+ int event_monitor_fd;
> >+
> >+ VIR_DEBUG("Event handler loop thread starting");
> >+
> >+ while ((event_monitor_fd = open(mon->eventmonitorpath, O_RDONLY)) < 0) {
> >+ if (errno == EINTR) {
> >+ g_usleep(100000); // 100 milli seconds
> >+ continue;
> >+ }
> >+ /*
> >+ * Any other error should be a BUG(kernel/libc/libvirtd)
> >+ * (ENOMEM can happen on exceeding per-user limits)
> >+ */
> >+ VIR_ERROR(_("Failed to open the event monitor FIFO(%1$s) read end!"),
> >+ mon->eventmonitorpath);
> >+ abort();
> >+ }
> >+ VIR_DEBUG("Opened the event monitor FIFO(%s)", mon->eventmonitorpath);
> >+
> >+ /*
> >+ * We would need to wait until VM is initialized.
> >+ */
> Does this comment apply to next while loop where the events are
> processed?
>
No, this applies to the immediate while loop where vm ref is obtained.
> >+ while (!(vm = virObjectRef(mon->vm)))
> >+ g_usleep(100000); // 100 milli seconds
>
> In this while loop you are only getting a reference to vm object.
>
Yes. At this point, we are not sure if mon->vm is initialized. The
parent thread initializes mon->vm only after starting this event handler
thread. Hence, wait until we have a vm ref.
> >+
> >+ while (g_atomic_int_get(&mon->event_handler_stop) == 0) {
> >+ VIR_DEBUG("Reading events from event monitor file...");
> >+ /* Read and process events here */
> >+ }
> >+
> >+ VIR_FORCE_CLOSE(event_monitor_fd);
>
> >+ virObjectUnref(vm);
> >+
> >+ VIR_DEBUG("Event handler loop thread exiting");
> >+ return;
> >+}
> >+
> >+int virCHStartEventHandler(virCHMonitor *mon)
> >+{
> >+ g_autofree char *name = NULL;
> >+ name = g_strdup_printf("event-handler-%d", mon->pid);
>
> It would be better to name the thread something similar to
> "event-handler_ch_${pid}_${vmname}". A similar format is
> used for cgroup naming.
>
> >+
> >+ virObjectRef(mon);
> >+ if (virThreadCreateFull(&mon->event_handler_thread,
> >+ false,
> >+ virCHEventHandlerLoop,
> >+ name,
> >+ false,
> >+ mon) < 0) {
> >+ virObjectUnref(mon);
> >+ return -1;
> >+ }
> >+ virObjectUnref(mon);
> >+
> >+ g_atomic_int_set(&mon->event_handler_stop, 0);
> >+ return 0;
> >+}
> >+
> >+void virCHStopEventHandler(virCHMonitor *mon)
> >+{
> >+ g_atomic_int_set(&mon->event_handler_stop, 1);
> >+}
> >diff --git a/src/ch/ch_events.h b/src/ch/ch_events.h
> >new file mode 100644
> >index 0000000000..4c8a48231d
> >--- /dev/null
> >+++ b/src/ch/ch_events.h
> >@@ -0,0 +1,26 @@
> >+/*
> >+ * Copyright Microsoft Corp. 2024
> >+ *
> >+ * ch_events.h: header file for handling Cloud-Hypervisor events
> >+ *
> >+ * This library is free software; you can redistribute it and/or
> >+ * modify it under the terms of the GNU Lesser General Public
> >+ * License as published by the Free Software Foundation; either
> >+ * version 2.1 of the License, or (at your option) any later version.
> >+ *
> >+ * This library is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> >+ * Lesser General Public License for more details.
> >+ *
> >+ * You should have received a copy of the GNU Lesser General Public
> >+ * License along with this library. If not, see
> >+ * <http://www.gnu.org/licenses/>.
> >+ */
> >+
> >+#pragma once
> >+
> >+#include "ch_monitor.h"
> >+
> >+int virCHStartEventHandler(virCHMonitor *mon);
> >+void virCHStopEventHandler(virCHMonitor *mon);
> >diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> >index 8c99fe1019..5e43bf9ef8 100644
> >--- a/src/ch/ch_monitor.c
> >+++ b/src/ch/ch_monitor.c
> >@@ -26,6 +26,7 @@
> > #include "datatypes.h"
> > #include "ch_conf.h"
> >+#include "ch_events.h"
> > #include "ch_interface.h"
> > #include "ch_monitor.h"
> > #include "domain_interface.h"
> >@@ -572,6 +573,28 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
> > return NULL;
> > }
> >+ /* Event monitor file to listen for VM state changes */
> >+ mon->eventmonitorpath = g_strdup_printf("%s/%s-event-monitor-fifo",
> >+ cfg->stateDir, vm->def->name);
> >+ if (virFileExists(mon->eventmonitorpath) &&
> >+ !virFileIsNamedPipe(mon->eventmonitorpath)) {
> >+ VIR_WARN("Monitor file (%s) is not a FIFO, trying to delete!",
> >+ mon->eventmonitorpath);
> >+ if (virFileRemove(mon->eventmonitorpath, -1, -1) < 0) {
> >+ virReportError(VIR_ERR_INTERNAL_ERROR,
> >+ _("Failed to remove the file: %1$s"),
> >+ mon->eventmonitorpath);
> >+ return NULL;
> >+ }
> >+ }
> >+
> This does not cover the case when eventmonitorpath exists, and is a
> named pipe, may be, not cleaned up from a previous run.
>
> You are better off just deleting the file and open a new one with
> mkfifo below.
>
> >+ if (mkfifo(mon->eventmonitorpath, S_IWUSR | S_IRUSR) < 0 &&
> >+ errno != EEXIST) {
> >+ virReportSystemError(errno, "%s",
> >+ _("Cannot create monitor FIFO"));
> >+ return NULL;
> >+ }
> >+
> > cmd = virCommandNew(vm->def->emulator);
> > virCommandSetUmask(cmd, 0x002);
> > socket_fd = chMonitorCreateSocket(mon->socketpath);
> >@@ -593,6 +616,9 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
> > if (virCommandRunAsync(cmd, &mon->pid) < 0)
> > return NULL;
> >+ if (virCHStartEventHandler(mon) < 0)
> >+ return NULL;
> >+
> > /* get a curl handle */
> > mon->handle = curl_easy_init();
> >@@ -641,6 +667,8 @@ void virCHMonitorClose(virCHMonitor *mon)
> > g_free(mon->eventmonitorpath);
> > }
> >+ virCHStopEventHandler(mon);
> >+
> Please move virCHStopEventHandler above `if (mon->eventmonitorpath) {`
> section. Doing so, will allow the event monitor thread to cleanly
> exit, before you invoke `virFileRemove(mon->eventmonitorpath...`
Thanks for this catch. I will move it.
And will also apply other suggestions.
>
> > virObjectUnref(mon);
> > }
> >diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h
> >index 2ef8706b99..878a185f29 100644
> >--- a/src/ch/ch_monitor.h
> >+++ b/src/ch/ch_monitor.h
> >@@ -97,6 +97,9 @@ struct _virCHMonitor {
> > char *eventmonitorpath;
> >+ virThread event_handler_thread;
> >+ int event_handler_stop;
> >+
> > pid_t pid;
> > virDomainObj *vm;
> >diff --git a/src/ch/meson.build b/src/ch/meson.build
> >index 633966aac7..684beac1f2 100644
> >--- a/src/ch/meson.build
> >+++ b/src/ch/meson.build
> >@@ -7,6 +7,8 @@ ch_driver_sources = [
> > 'ch_domain.h',
> > 'ch_driver.c',
> > 'ch_driver.h',
> >+ 'ch_events.c',
> >+ 'ch_driver.h',
> > 'ch_interface.c',
> > 'ch_interface.h',
> > 'ch_monitor.c',
>
>
> --
> Regards,
> Praveen K Paladugu
Regards,
Purna Pavan Chandra
On 9/24/2024 8:22 AM, Purna Pavan Chandra Aekkaladevi wrote:
> On Mon, Sep 23, 2024 at 04:29:35PM -0500, Praveen K Paladugu wrote:
>>
>>
>> On 9/19/2024 8:02 AM, Purna Pavan Chandra Aekkaladevi wrote:
>>> Use a FIFO(named pipe) for --event-monitor option in CH. Introduce a new
>>> thread, `virCHEventHandlerLoop`, to continuously monitor and handle
>>> events from cloud-hypervisor.
>>>
>>> Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@linux.microsoft.com>
>>> Co-authored-by: Vineeth Pillai <viremana@linux.microsoft.com>
>>> ---
>>> po/POTFILES | 1 +
>>> src/ch/ch_events.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
>>> src/ch/ch_events.h | 26 ++++++++++++
>>> src/ch/ch_monitor.c | 28 +++++++++++++
>>> src/ch/ch_monitor.h | 3 ++
>>> src/ch/meson.build | 2 +
>>> 6 files changed, 156 insertions(+)
>>> create mode 100644 src/ch/ch_events.c
>>> create mode 100644 src/ch/ch_events.h
>>>
>>> diff --git a/po/POTFILES b/po/POTFILES
>>> index 1ed4086d2c..d53307cec4 100644
>>> --- a/po/POTFILES
>>> +++ b/po/POTFILES
>>> @@ -21,6 +21,7 @@ src/bhyve/bhyve_process.c
>>> src/ch/ch_conf.c
>>> src/ch/ch_domain.c
>>> src/ch/ch_driver.c
>>> +src/ch/ch_events.c
>>> src/ch/ch_interface.c
>>> src/ch/ch_monitor.c
>>> src/ch/ch_process.c
>>> diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c
>>> new file mode 100644
>>> index 0000000000..851fbc9f26
>>> --- /dev/null
>>> +++ b/src/ch/ch_events.c
>>> @@ -0,0 +1,96 @@
>>> +/*
>>> + * Copyright Microsoft Corp. 2024
>>> + *
>>> + * ch_events.h: Handle Cloud-Hypervisor events
>>
>> please fix the filename here.
>>
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library. If not, see
>>> + * <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include <config.h>
>>> +
>>> +#include <fcntl.h>
>>> +
>>> +#include "ch_events.h"
>>> +#include "virfile.h"
>>> +#include "virlog.h"
>>> +
>>> +VIR_LOG_INIT("ch.ch_events");
>>> +
>>> +static void virCHEventHandlerLoop(void *data)
>>> +{
>>> + virCHMonitor *mon = data;
>>> + virDomainObj *vm = NULL;
>>> + int event_monitor_fd;
>>> +
>>> + VIR_DEBUG("Event handler loop thread starting");
>>> +
>>> + while ((event_monitor_fd = open(mon->eventmonitorpath, O_RDONLY)) < 0) {
>>> + if (errno == EINTR) {
>>> + g_usleep(100000); // 100 milli seconds
>>> + continue;
>>> + }
>>> + /*
>>> + * Any other error should be a BUG(kernel/libc/libvirtd)
>>> + * (ENOMEM can happen on exceeding per-user limits)
>>> + */
>>> + VIR_ERROR(_("Failed to open the event monitor FIFO(%1$s) read end!"),
>>> + mon->eventmonitorpath);
>>> + abort();
>>> + }
>>> + VIR_DEBUG("Opened the event monitor FIFO(%s)", mon->eventmonitorpath);
>>> +
>>> + /*
>>> + * We would need to wait until VM is initialized.
>>> + */
>> Does this comment apply to next while loop where the events are
>> processed?
>>
>
> No, this applies to the immediate while loop where vm ref is obtained.
>
>>> + while (!(vm = virObjectRef(mon->vm)))
>>> + g_usleep(100000); // 100 milli seconds
>>
>> In this while loop you are only getting a reference to vm object.
>>
>
> Yes. At this point, we are not sure if mon->vm is initialized. The
> parent thread initializes mon->vm only after starting this event handler
> thread. Hence, wait until we have a vm ref.
Why do you need to wait here? Is there a race condition here?
If yes, will just waiting for a reference for vm object enough?
>
>>> +
>>> + while (g_atomic_int_get(&mon->event_handler_stop) == 0) {
>>> + VIR_DEBUG("Reading events from event monitor file...");
>>> + /* Read and process events here */
>>> + }
>>> +
>>> + VIR_FORCE_CLOSE(event_monitor_fd);
>>
>>> + virObjectUnref(vm);
>>> +
>>> + VIR_DEBUG("Event handler loop thread exiting");
>>> + return;
>>> +}
>>> +
>>> +int virCHStartEventHandler(virCHMonitor *mon)
>>> +{
>>> + g_autofree char *name = NULL;
>>> + name = g_strdup_printf("event-handler-%d", mon->pid);
>>
>> It would be better to name the thread something similar to
>> "event-handler_ch_${pid}_${vmname}". A similar format is
>> used for cgroup naming.
>>
>>> +
>>> + virObjectRef(mon);
>>> + if (virThreadCreateFull(&mon->event_handler_thread,
>>> + false,
>>> + virCHEventHandlerLoop,
>>> + name,
>>> + false,
>>> + mon) < 0) {
>>> + virObjectUnref(mon);
>>> + return -1;
>>> + }
>>> + virObjectUnref(mon);
>>> +
>>> + g_atomic_int_set(&mon->event_handler_stop, 0);
>>> + return 0;
>>> +}
>>> +
>>> +void virCHStopEventHandler(virCHMonitor *mon)
>>> +{
>>> + g_atomic_int_set(&mon->event_handler_stop, 1);
>>> +}
>>> diff --git a/src/ch/ch_events.h b/src/ch/ch_events.h
>>> new file mode 100644
>>> index 0000000000..4c8a48231d
>>> --- /dev/null
>>> +++ b/src/ch/ch_events.h
>>> @@ -0,0 +1,26 @@
>>> +/*
>>> + * Copyright Microsoft Corp. 2024
>>> + *
>>> + * ch_events.h: header file for handling Cloud-Hypervisor events
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library. If not, see
>>> + * <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#pragma once
>>> +
>>> +#include "ch_monitor.h"
>>> +
>>> +int virCHStartEventHandler(virCHMonitor *mon);
>>> +void virCHStopEventHandler(virCHMonitor *mon);
>>> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
>>> index 8c99fe1019..5e43bf9ef8 100644
>>> --- a/src/ch/ch_monitor.c
>>> +++ b/src/ch/ch_monitor.c
>>> @@ -26,6 +26,7 @@
>>> #include "datatypes.h"
>>> #include "ch_conf.h"
>>> +#include "ch_events.h"
>>> #include "ch_interface.h"
>>> #include "ch_monitor.h"
>>> #include "domain_interface.h"
>>> @@ -572,6 +573,28 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
>>> return NULL;
>>> }
>>> + /* Event monitor file to listen for VM state changes */
>>> + mon->eventmonitorpath = g_strdup_printf("%s/%s-event-monitor-fifo",
>>> + cfg->stateDir, vm->def->name);
>>> + if (virFileExists(mon->eventmonitorpath) &&
>>> + !virFileIsNamedPipe(mon->eventmonitorpath)) {
>>> + VIR_WARN("Monitor file (%s) is not a FIFO, trying to delete!",
>>> + mon->eventmonitorpath);
>>> + if (virFileRemove(mon->eventmonitorpath, -1, -1) < 0) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("Failed to remove the file: %1$s"),
>>> + mon->eventmonitorpath);
>>> + return NULL;
>>> + }
>>> + }
>>> +
>> This does not cover the case when eventmonitorpath exists, and is a
>> named pipe, may be, not cleaned up from a previous run.
>>
>> You are better off just deleting the file and open a new one with
>> mkfifo below.
>>
>>> + if (mkfifo(mon->eventmonitorpath, S_IWUSR | S_IRUSR) < 0 &&
>>> + errno != EEXIST) {
>>> + virReportSystemError(errno, "%s",
>>> + _("Cannot create monitor FIFO"));
>>> + return NULL;
>>> + }
>>> +
>>> cmd = virCommandNew(vm->def->emulator);
>>> virCommandSetUmask(cmd, 0x002);
>>> socket_fd = chMonitorCreateSocket(mon->socketpath);
>>> @@ -593,6 +616,9 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
>>> if (virCommandRunAsync(cmd, &mon->pid) < 0)
>>> return NULL;
>>> + if (virCHStartEventHandler(mon) < 0)
>>> + return NULL;
>>> +
>>> /* get a curl handle */
>>> mon->handle = curl_easy_init();
>>> @@ -641,6 +667,8 @@ void virCHMonitorClose(virCHMonitor *mon)
>>> g_free(mon->eventmonitorpath);
>>> }
>>> + virCHStopEventHandler(mon);
>>> +
>> Please move virCHStopEventHandler above `if (mon->eventmonitorpath) {`
>> section. Doing so, will allow the event monitor thread to cleanly
>> exit, before you invoke `virFileRemove(mon->eventmonitorpath...`
>
> Thanks for this catch. I will move it.
> And will also apply other suggestions.
>
>>
>>> virObjectUnref(mon);
>>> }
>>> diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h
>>> index 2ef8706b99..878a185f29 100644
>>> --- a/src/ch/ch_monitor.h
>>> +++ b/src/ch/ch_monitor.h
>>> @@ -97,6 +97,9 @@ struct _virCHMonitor {
>>> char *eventmonitorpath;
>>> + virThread event_handler_thread;
>>> + int event_handler_stop;
>>> +
>>> pid_t pid;
>>> virDomainObj *vm;
>>> diff --git a/src/ch/meson.build b/src/ch/meson.build
>>> index 633966aac7..684beac1f2 100644
>>> --- a/src/ch/meson.build
>>> +++ b/src/ch/meson.build
>>> @@ -7,6 +7,8 @@ ch_driver_sources = [
>>> 'ch_domain.h',
>>> 'ch_driver.c',
>>> 'ch_driver.h',
>>> + 'ch_events.c',
>>> + 'ch_driver.h',
>>> 'ch_interface.c',
>>> 'ch_interface.h',
>>> 'ch_monitor.c',
>>
>>
>> --
>> Regards,
>> Praveen K Paladugu
>
> Regards,
> Purna Pavan Chandra
>
--
Regards,
Praveen K Paladugu
On Thu, Sep 26, 2024 at 11:11:54AM -0500, Praveen K Paladugu wrote:
>
>
> On 9/24/2024 8:22 AM, Purna Pavan Chandra Aekkaladevi wrote:
> > On Mon, Sep 23, 2024 at 04:29:35PM -0500, Praveen K Paladugu wrote:
> > >
> > >
> > > On 9/19/2024 8:02 AM, Purna Pavan Chandra Aekkaladevi wrote:
> > > > Use a FIFO(named pipe) for --event-monitor option in CH. Introduce a new
> > > > thread, `virCHEventHandlerLoop`, to continuously monitor and handle
> > > > events from cloud-hypervisor.
> > > >
> > > > Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@linux.microsoft.com>
> > > > Co-authored-by: Vineeth Pillai <viremana@linux.microsoft.com>
> > > > ---
> > > > po/POTFILES | 1 +
> > > > src/ch/ch_events.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
> > > > src/ch/ch_events.h | 26 ++++++++++++
> > > > src/ch/ch_monitor.c | 28 +++++++++++++
> > > > src/ch/ch_monitor.h | 3 ++
> > > > src/ch/meson.build | 2 +
> > > > 6 files changed, 156 insertions(+)
> > > > create mode 100644 src/ch/ch_events.c
> > > > create mode 100644 src/ch/ch_events.h
> > > >
> > > > diff --git a/po/POTFILES b/po/POTFILES
> > > > index 1ed4086d2c..d53307cec4 100644
> > > > --- a/po/POTFILES
> > > > +++ b/po/POTFILES
> > > > @@ -21,6 +21,7 @@ src/bhyve/bhyve_process.c
> > > > src/ch/ch_conf.c
> > > > src/ch/ch_domain.c
> > > > src/ch/ch_driver.c
> > > > +src/ch/ch_events.c
> > > > src/ch/ch_interface.c
> > > > src/ch/ch_monitor.c
> > > > src/ch/ch_process.c
> > > > diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c
> > > > new file mode 100644
> > > > index 0000000000..851fbc9f26
> > > > --- /dev/null
> > > > +++ b/src/ch/ch_events.c
> > > > @@ -0,0 +1,96 @@
> > > > +/*
> > > > + * Copyright Microsoft Corp. 2024
> > > > + *
> > > > + * ch_events.h: Handle Cloud-Hypervisor events
> > >
> > > please fix the filename here.
Will fix it in V2.
> > >
> > > > + *
> > > > + * This library is free software; you can redistribute it and/or
> > > > + * modify it under the terms of the GNU Lesser General Public
> > > > + * License as published by the Free Software Foundation; either
> > > > + * version 2.1 of the License, or (at your option) any later version.
> > > > + *
> > > > + * This library is distributed in the hope that it will be useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > > + * Lesser General Public License for more details.
> > > > + *
> > > > + * You should have received a copy of the GNU Lesser General Public
> > > > + * License along with this library. If not, see
> > > > + * <http://www.gnu.org/licenses/>.
> > > > + */
> > > > +
> > > > +#include <config.h>
> > > > +
> > > > +#include <fcntl.h>
> > > > +
> > > > +#include "ch_events.h"
> > > > +#include "virfile.h"
> > > > +#include "virlog.h"
> > > > +
> > > > +VIR_LOG_INIT("ch.ch_events");
> > > > +
> > > > +static void virCHEventHandlerLoop(void *data)
> > > > +{
> > > > + virCHMonitor *mon = data;
> > > > + virDomainObj *vm = NULL;
> > > > + int event_monitor_fd;
> > > > +
> > > > + VIR_DEBUG("Event handler loop thread starting");
> > > > +
> > > > + while ((event_monitor_fd = open(mon->eventmonitorpath, O_RDONLY)) < 0) {
> > > > + if (errno == EINTR) {
> > > > + g_usleep(100000); // 100 milli seconds
> > > > + continue;
> > > > + }
> > > > + /*
> > > > + * Any other error should be a BUG(kernel/libc/libvirtd)
> > > > + * (ENOMEM can happen on exceeding per-user limits)
> > > > + */
> > > > + VIR_ERROR(_("Failed to open the event monitor FIFO(%1$s) read end!"),
> > > > + mon->eventmonitorpath);
> > > > + abort();
> > > > + }
> > > > + VIR_DEBUG("Opened the event monitor FIFO(%s)", mon->eventmonitorpath);
> > > > +
> > > > + /*
> > > > + * We would need to wait until VM is initialized.
> > > > + */
> > > Does this comment apply to next while loop where the events are
> > > processed?
> > >
> >
> > No, this applies to the immediate while loop where vm ref is obtained.
> >
> > > > + while (!(vm = virObjectRef(mon->vm)))
> > > > + g_usleep(100000); // 100 milli seconds
> > >
> > > In this while loop you are only getting a reference to vm object.
> > >
> >
> > Yes. At this point, we are not sure if mon->vm is initialized. The
> > parent thread initializes mon->vm only after starting this event handler
> > thread. Hence, wait until we have a vm ref.
>
> Why do you need to wait here? Is there a race condition here?
> If yes, will just waiting for a reference for vm object enough?
>
This is actually not needed if mon->vm is initialized before started
this thread. So will do that and get rid of this loop in V2.
>
> >
> > > > +
> > > > + while (g_atomic_int_get(&mon->event_handler_stop) == 0) {
> > > > + VIR_DEBUG("Reading events from event monitor file...");
> > > > + /* Read and process events here */
> > > > + }
> > > > +
> > > > + VIR_FORCE_CLOSE(event_monitor_fd);
> > >
> > > > + virObjectUnref(vm);
> > > > +
> > > > + VIR_DEBUG("Event handler loop thread exiting");
> > > > + return;
> > > > +}
> > > > +
> > > > +int virCHStartEventHandler(virCHMonitor *mon)
> > > > +{
> > > > + g_autofree char *name = NULL;
> > > > + name = g_strdup_printf("event-handler-%d", mon->pid);
> > >
> > > It would be better to name the thread something similar to
> > > "event-handler_ch_${pid}_${vmname}". A similar format is
> > > used for cgroup naming.
> > >
Since there is a contraint on the length of thread name, I will follow
the convention of `ch-evt-${pid}`
> > > > +
> > > > + virObjectRef(mon);
> > > > + if (virThreadCreateFull(&mon->event_handler_thread,
> > > > + false,
> > > > + virCHEventHandlerLoop,
> > > > + name,
> > > > + false,
> > > > + mon) < 0) {
> > > > + virObjectUnref(mon);
> > > > + return -1;
> > > > + }
> > > > + virObjectUnref(mon);
> > > > +
> > > > + g_atomic_int_set(&mon->event_handler_stop, 0);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +void virCHStopEventHandler(virCHMonitor *mon)
> > > > +{
> > > > + g_atomic_int_set(&mon->event_handler_stop, 1);
> > > > +}
> > > > diff --git a/src/ch/ch_events.h b/src/ch/ch_events.h
> > > > new file mode 100644
> > > > index 0000000000..4c8a48231d
> > > > --- /dev/null
> > > > +++ b/src/ch/ch_events.h
> > > > @@ -0,0 +1,26 @@
> > > > +/*
> > > > + * Copyright Microsoft Corp. 2024
> > > > + *
> > > > + * ch_events.h: header file for handling Cloud-Hypervisor events
> > > > + *
> > > > + * This library is free software; you can redistribute it and/or
> > > > + * modify it under the terms of the GNU Lesser General Public
> > > > + * License as published by the Free Software Foundation; either
> > > > + * version 2.1 of the License, or (at your option) any later version.
> > > > + *
> > > > + * This library is distributed in the hope that it will be useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > > + * Lesser General Public License for more details.
> > > > + *
> > > > + * You should have received a copy of the GNU Lesser General Public
> > > > + * License along with this library. If not, see
> > > > + * <http://www.gnu.org/licenses/>.
> > > > + */
> > > > +
> > > > +#pragma once
> > > > +
> > > > +#include "ch_monitor.h"
> > > > +
> > > > +int virCHStartEventHandler(virCHMonitor *mon);
> > > > +void virCHStopEventHandler(virCHMonitor *mon);
> > > > diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> > > > index 8c99fe1019..5e43bf9ef8 100644
> > > > --- a/src/ch/ch_monitor.c
> > > > +++ b/src/ch/ch_monitor.c
> > > > @@ -26,6 +26,7 @@
> > > > #include "datatypes.h"
> > > > #include "ch_conf.h"
> > > > +#include "ch_events.h"
> > > > #include "ch_interface.h"
> > > > #include "ch_monitor.h"
> > > > #include "domain_interface.h"
> > > > @@ -572,6 +573,28 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
> > > > return NULL;
> > > > }
> > > > + /* Event monitor file to listen for VM state changes */
> > > > + mon->eventmonitorpath = g_strdup_printf("%s/%s-event-monitor-fifo",
> > > > + cfg->stateDir, vm->def->name);
> > > > + if (virFileExists(mon->eventmonitorpath) &&
> > > > + !virFileIsNamedPipe(mon->eventmonitorpath)) {
> > > > + VIR_WARN("Monitor file (%s) is not a FIFO, trying to delete!",
> > > > + mon->eventmonitorpath);
> > > > + if (virFileRemove(mon->eventmonitorpath, -1, -1) < 0) {
> > > > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > > > + _("Failed to remove the file: %1$s"),
> > > > + mon->eventmonitorpath);
> > > > + return NULL;
> > > > + }
> > > > + }
> > > > +
> > > This does not cover the case when eventmonitorpath exists, and is a
> > > named pipe, may be, not cleaned up from a previous run.
> > >
> > > You are better off just deleting the file and open a new one with
> > > mkfifo below.
> > >
Will take care in V2.
> > > > + if (mkfifo(mon->eventmonitorpath, S_IWUSR | S_IRUSR) < 0 &&
> > > > + errno != EEXIST) {
> > > > + virReportSystemError(errno, "%s",
> > > > + _("Cannot create monitor FIFO"));
> > > > + return NULL;
> > > > + }
> > > > +
> > > > cmd = virCommandNew(vm->def->emulator);
> > > > virCommandSetUmask(cmd, 0x002);
> > > > socket_fd = chMonitorCreateSocket(mon->socketpath);
> > > > @@ -593,6 +616,9 @@ virCHMonitorNew(virDomainObj *vm, virCHDriverConfig *cfg)
> > > > if (virCommandRunAsync(cmd, &mon->pid) < 0)
> > > > return NULL;
> > > > + if (virCHStartEventHandler(mon) < 0)
> > > > + return NULL;
> > > > +
> > > > /* get a curl handle */
> > > > mon->handle = curl_easy_init();
> > > > @@ -641,6 +667,8 @@ void virCHMonitorClose(virCHMonitor *mon)
> > > > g_free(mon->eventmonitorpath);
> > > > }
> > > > + virCHStopEventHandler(mon);
> > > > +
> > > Please move virCHStopEventHandler above `if (mon->eventmonitorpath) {`
> > > section. Doing so, will allow the event monitor thread to cleanly
> > > exit, before you invoke `virFileRemove(mon->eventmonitorpath...`
> >
> > Thanks for this catch. I will move it.
> > And will also apply other suggestions.
> >
> > >
> > > > virObjectUnref(mon);
> > > > }
> > > > diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h
> > > > index 2ef8706b99..878a185f29 100644
> > > > --- a/src/ch/ch_monitor.h
> > > > +++ b/src/ch/ch_monitor.h
> > > > @@ -97,6 +97,9 @@ struct _virCHMonitor {
> > > > char *eventmonitorpath;
> > > > + virThread event_handler_thread;
> > > > + int event_handler_stop;
> > > > +
> > > > pid_t pid;
> > > > virDomainObj *vm;
> > > > diff --git a/src/ch/meson.build b/src/ch/meson.build
> > > > index 633966aac7..684beac1f2 100644
> > > > --- a/src/ch/meson.build
> > > > +++ b/src/ch/meson.build
> > > > @@ -7,6 +7,8 @@ ch_driver_sources = [
> > > > 'ch_domain.h',
> > > > 'ch_driver.c',
> > > > 'ch_driver.h',
> > > > + 'ch_events.c',
> > > > + 'ch_driver.h',
> > > > 'ch_interface.c',
> > > > 'ch_interface.h',
> > > > 'ch_monitor.c',
> > >
> > >
> > > --
> > > Regards,
> > > Praveen K Paladugu
> >
> > Regards,
> > Purna Pavan Chandra
> >
>
> --
> Regards,
> Praveen K Paladugu
Regards,
Purna Pavan Chandra
© 2016 - 2026 Red Hat, Inc.