fix: cope with failure checking client status#2298
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent script/rvd_back from crashing during nightly hibernation when verifying a VM’s viewer connection status, addressing issue #2293.
Changes:
- Wraps
client_status(1)calls in_verify_connectionwith anevalblock. - Defaults status to
'disconnected'on failure and emits a warning when an exception occurs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| eval { | ||
| $status = $domain->client_status(1); | ||
| }; | ||
| warn "Error checking ".$domain_f->name." $@" if $@; |
There was a problem hiding this comment.
The issue report indicates a segmentation fault (core dump). Wrapping client_status in eval will only catch Perl exceptions (die/confess) and will not prevent a true segfault in XS/C code; the process would still crash. If the crash is a real segfault, consider moving the status check into a separate child process (fork/IPC) or fixing/guarding the underlying XS call path so a failure can be handled without taking down rvd_back.
| eval { | ||
| $status = $domain->client_status(1); | ||
| }; | ||
| warn "Error checking ".$domain_f->name." $@" if $@; |
There was a problem hiding this comment.
On exception, this code leaves $status as 'disconnected', which makes _verify_connection likely return 0 and allow shutdown/hibernate to proceed even though the connection status could not be verified. To avoid mistakenly acting on a VM when the check is failing, consider treating an exception as "unknown/connected" (eg return 1 to dismiss shutdown) or short-circuiting to skip the domain with a warning so the caller doesn’t interpret a failed check as a confirmed disconnect.
| warn "Error checking ".$domain_f->name." $@" if $@; | |
| if ($@) { | |
| warn "Error checking ".$domain_f->name." $@"; | |
| print "\n" if $VERBOSE; | |
| # Treat exceptions as "unknown/connected" to avoid unsafe shutdown. | |
| return 1; | |
| } |
| my $status = 'disconnected'; | ||
| eval { | ||
| $status = $domain->client_status(1); | ||
| }; | ||
| warn "Error checking ".$domain_f->name." $@" if $@; |
There was a problem hiding this comment.
warn "Error checking ... $@" is inside the retry loop, so a persistent failure will emit up to $TIME_CONNECTION warnings per domain; also $@ is global and can leak/clobber other error handling. Consider localizing $@ (eg local $@;) and only warning once per domain (or only under $VERBOSE) to keep logs actionable.
closes #2293