[PATCHv2 0/2] ch: minor fixes - segfault and last meaningful error preservation
During fixing some other issue found out a few minor bugs related to CH driver. First bug is related to monitor object ref count in virCHStartEventHandler and virCHEventHandlerLoop, as object was unrefed by parent while is still used by child thread. Moved object unref to the correct place to the correct thread. Second bug is related to v object ref count in virCHStartEventHandler, as vm object was unrefed, while is still used later, move object unref to after its last usage. Third bug is related to error handling and propagation. Last meaninful error was reset by virCHProcessStop, which have led to unknown error message in virsh. Add error preservation at the begining of the function and restoring it at the end to keep consistent error output Kirill Shchetiniuk (2): ch: virCHProcessStop preserve last meaningful error ch: ref count fix in virCHEventHandlerLoop and virCHStartEventHandler src/ch/ch_events.c | 4 ++-- src/ch/ch_process.c | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) -- 2.48.1
The last meaningful error was being reset during the execution of virCHProcessStop, which led to inconsistent error output in case any error occurred before the stop. To maintain consistent error output and avoid unknown error messages, last meaningful error preservation has been added. Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/ch/ch_process.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 9a85f7869e..9388d5bd83 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -995,11 +995,14 @@ virCHProcessStop(virCHDriver *driver, virCHDomainObjPrivate *priv = vm->privateData; virCHDriverConfig *cfg = virCHDriverGetConfig(driver); virDomainDef *def = vm->def; + virErrorPtr orig_err = NULL; size_t i; VIR_DEBUG("Stopping VM name=%s pid=%d reason=%d", vm->def->name, (int)vm->pid, (int)reason); + virErrorPreserveLast(&orig_err); + if (priv->monitor) { g_clear_pointer(&priv->monitor, virCHMonitorClose); } @@ -1032,6 +1035,8 @@ virCHProcessStop(virCHDriver *driver, virHostdevReAttachDomainDevices(driver->hostdevMgr, CH_DRIVER_NAME, def, hostdev_flags); + + virErrorRestore(&orig_err); return 0; } -- 2.48.1
Monitor Reference Counting Fix: The monitor was being unreferenced inside virCHEventHandlerLoop, that has led to a segmentation fault. The monitor was unreferenced after successful child thread creation while it was still being used in the child thread. To maintain correct reference counting, the monitor unreference has been moved to virCHStartEventHandler (the child thread). Other monitor unreference in virCHStartEventHandler are retained for cases where new thread creation fails, as the reference in not used afterward. VM Reference Counting Fix: The VM object was being unreferenced in virCHEventHandlerLoop while it was still being used by the debug print. To resolve this, the VM unreference has been moved to after the debug print. Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/ch/ch_events.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c index 1cce30836a..2dd3e7ecc2 100644 --- a/src/ch/ch_events.c +++ b/src/ch/ch_events.c @@ -287,8 +287,9 @@ virCHEventHandlerLoop(void *data) } g_clear_pointer(&mon->event_buffer.buffer, g_free); - virObjectUnref(vm); VIR_DEBUG("%s: Event handler loop thread exiting", vm->def->name); + virObjectUnref(vm); + virObjectUnref(mon); return; } @@ -308,7 +309,6 @@ virCHStartEventHandler(virCHMonitor *mon) virObjectUnref(mon); return -1; } - virObjectUnref(mon); g_atomic_int_set(&mon->event_handler_stop, 0); return 0; -- 2.48.1
On 3/14/25 12:51, Kirill Shchetiniuk wrote:
During fixing some other issue found out a few minor bugs related to CH driver.
First bug is related to monitor object ref count in virCHStartEventHandler and virCHEventHandlerLoop, as object was unrefed by parent while is still used by child thread. Moved object unref to the correct place to the correct thread.
Second bug is related to v object ref count in virCHStartEventHandler, as vm object was unrefed, while is still used later, move object unref to after its last usage.
Third bug is related to error handling and propagation. Last meaninful error was reset by virCHProcessStop, which have led to unknown error message in virsh. Add error preservation at the begining of the function and restoring it at the end to keep consistent error output
Kirill Shchetiniuk (2): ch: virCHProcessStop preserve last meaningful error ch: ref count fix in virCHEventHandlerLoop and virCHStartEventHandler
src/ch/ch_events.c | 4 ++-- src/ch/ch_process.c | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-)
I've reworded commit messages a bit and merged. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Congratulations on your first libvirt contribution! Michal
participants (2)
-
Kirill Shchetiniuk -
Michal Prívozník