11:11 a.m.
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(a)linux.microsoft.com>
>> Co-authored-by: Vineeth Pillai <viremana(a)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