[PATCH 0/2] ch: minor fixes - segfault fix and error preserving
*** BLURB HERE *** Kirill Shchetiniuk (2): ch: memory (segmentation fault) fix ch: preserve last error before stop fix src/ch/ch_events.c | 6 +++--- src/ch/ch_process.c | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) -- 2.48.1
Move monitor object unreference from virCHStartEventHandler to virCHEventHandlerLoop. Put VM unreference after debug print in virCHEventHandlerLoop. Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/ch/ch_events.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c index 1cce30836a..20e7fbd705 100644 --- a/src/ch/ch_events.c +++ b/src/ch/ch_events.c @@ -286,9 +286,10 @@ virCHEventHandlerLoop(void *data) } } - g_clear_pointer(&mon->event_buffer.buffer, g_free); - virObjectUnref(vm); + g_clear_pointer(&(mon->event_buffer.buffer), g_free); 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 Thu, Mar 06, 2025 at 11:27:07 +0100, Kirill Shchetiniuk wrote:
Move monitor object unreference from virCHStartEventHandler to virCHEventHandlerLoop. Put VM unreference after debug print in virCHEventHandlerLoop.
Please use the commit message mainly to describe the problem you're fixiing. You only described what you changed; not why.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/ch/ch_events.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/ch/ch_events.c b/src/ch/ch_events.c index 1cce30836a..20e7fbd705 100644 --- a/src/ch/ch_events.c +++ b/src/ch/ch_events.c @@ -286,9 +286,10 @@ virCHEventHandlerLoop(void *data) } }
- g_clear_pointer(&mon->event_buffer.buffer, g_free);
Okay so ...
- virObjectUnref(vm); + g_clear_pointer(&(mon->event_buffer.buffer), g_free);
... this was taking reference of 'mon' instead of the buffer.
VIR_DEBUG("%s: Event handler loop thread exiting", vm->def->name);
and this debug technically accesses 'vm' which you no longer have reference for ...
+ virObjectUnref(vm); + virObjectUnref(mon);
But that doesn't explain why this is needed.
return; }
@@ -308,7 +309,6 @@ virCHStartEventHandler(virCHMonitor *mon) virObjectUnref(mon);
and it is weird because you do unref it here ...
return -1; } - virObjectUnref(mon);
... but remove it from here.
g_atomic_int_set(&mon->event_handler_stop, 0); return 0;
Please use the commit message mainly to describe the problem you're fixiing. You only described what you changed; not why.
Thank you for your feedback, next time will keep it mind.
and it is weird because you do unref it here ...
This unref executes in case of any failure during new thread start up, as it wont be used later, otherwise this reference is used by virCHEventHandlerLoop thread and should be unrefed when virCHEventHandlerLoop is finished.
... but remove it from here.
In case of successful startup we do not want to unref an object cause it will be used in other thread.
Add last meaningful error preservation to virCHProcessStop. Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/ch/ch_process.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 9a85f7869e..38d466eaeb 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -995,11 +995,15 @@ 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); + /* Preserve origin error */ + virErrorPreserveLast(&orig_err); + if (priv->monitor) { g_clear_pointer(&priv->monitor, virCHMonitorClose); } @@ -1032,6 +1036,8 @@ virCHProcessStop(virCHDriver *driver, virHostdevReAttachDomainDevices(driver->hostdevMgr, CH_DRIVER_NAME, def, hostdev_flags); + + virErrorRestore(&orig_err); return 0; } -- 2.48.1
On Thu, Mar 06, 2025 at 11:27:08 +0100, Kirill Shchetiniuk wrote:
Add last meaningful error preservation to virCHProcessStop.
Signed-off-by: Kirill Shchetiniuk <kshcheti@redhat.com> --- src/ch/ch_process.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 9a85f7869e..38d466eaeb 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -995,11 +995,15 @@ 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);
+ /* Preserve origin error */
It's quite self-evident from the function called ;)
+ virErrorPreserveLast(&orig_err); + if (priv->monitor) { g_clear_pointer(&priv->monitor, virCHMonitorClose); } @@ -1032,6 +1036,8 @@ virCHProcessStop(virCHDriver *driver,
virHostdevReAttachDomainDevices(driver->hostdevMgr, CH_DRIVER_NAME, def, hostdev_flags); + + virErrorRestore(&orig_err); return 0; }
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (3)
-
Kirill Shchetiniuk -
kshcheti＠redhat.com
-
Peter Krempa