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@xxxxxxxxxxxxxxxxxxx> > >Co-authored-by: Vineeth Pillai <viremana@xxxxxxxxxxxxxxxxxxx> > >--- > > 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
