Copilot commented on code in PR #12872:
URL: https://github.com/apache/cloudstack/pull/12872#discussion_r2989366685
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -221,6 +223,20 @@ mount_operation() {
cleanup() {
local status=0
+ # Resume the VM if it was paused (e.g. by virsh backup-begin)
+ if [[ -n "$VM" ]]; then
+ local vm_state
+ vm_state=$(virsh -c qemu:///system domstate "$VM" 2>/dev/null)
Review Comment:
With `set -eo pipefail` enabled, `vm_state=$(virsh ... domstate ...)` will
cause `cleanup()` to exit immediately if `virsh domstate` returns non-zero
(e.g., VM not found / libvirt transient error), which can prevent
unmount/removal and also skip the resume attempt. Please make the `domstate`
probe non-fatal (e.g., allow failure and treat state as empty) so cleanup
always completes best-effort.
```suggestion
vm_state=$(virsh -c qemu:///system domstate "$VM" 2>/dev/null || true)
```
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -221,6 +223,20 @@ mount_operation() {
cleanup() {
local status=0
+ # Resume the VM if it was paused (e.g. by virsh backup-begin)
+ if [[ -n "$VM" ]]; then
+ local vm_state
+ vm_state=$(virsh -c qemu:///system domstate "$VM" 2>/dev/null)
+ if [[ "$vm_state" == "paused" ]]; then
Review Comment:
`virsh domstate` output can include a reason suffix (e.g., `paused
(ioerror)`), so an exact comparison to `"paused"` may miss paused VMs and fail
to resume them. Consider matching a `paused` prefix (or use `domstate --reason`
and parse the first token) so paused VMs are reliably detected.
```suggestion
if [[ "$vm_state" == paused* ]]; then
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
