Copilot commented on code in PR #13424:
URL: https://github.com/apache/cloudstack/pull/13424#discussion_r3459678170
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -196,10 +196,12 @@ backup_running_vm() {
# Print statistics
virsh -c qemu:///system domjobinfo $VM --completed
- du -sb $dest | cut -f1
-
- umount $mount_point
- rmdir $mount_point
+ backup_size=$(du -sb "$dest" 2>/dev/null | cut -f1) || true
+
+ timeout 60 umount "$mount_point" 2>/dev/null || true
+ rmdir "$mount_point" 2>/dev/null || true
Review Comment:
These `2>/dev/null || true` changes fully suppress failures, which can leave
behind mounted filesystems or temp directories without any indication
(especially if `timeout` triggers). Consider logging a warning when `du`,
`umount`, or `rmdir` fails (while still not failing the job), and consider
setting a safe default for `backup_size` (e.g., `0`) if `du` fails to avoid
emitting an empty value.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -196,10 +196,12 @@ backup_running_vm() {
# Print statistics
virsh -c qemu:///system domjobinfo $VM --completed
- du -sb $dest | cut -f1
-
- umount $mount_point
- rmdir $mount_point
+ backup_size=$(du -sb "$dest" 2>/dev/null | cut -f1) || true
+
+ timeout 60 umount "$mount_point" 2>/dev/null || true
+ rmdir "$mount_point" 2>/dev/null || true
+
+ echo -n "$backup_size"
Review Comment:
Switching from `du ... | cut -f1` (which emits a trailing newline) to `echo
-n` changes the output format and can break any caller that reads the size
line-by-line. Prefer preserving the newline (e.g., `echo \"$backup_size\"`) to
keep behavior consistent while still suppressing errors.
--
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]
