Copilot commented on code in PR #13424:
URL: https://github.com/apache/cloudstack/pull/13424#discussion_r3470255127


##########
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>$logFile | cut -f1) || backup_size=0

Review Comment:
   Redirection `2>$logFile` truncates `/var/log/cloudstack/agent/agent.log` 
every time this runs (even when `du` succeeds, because the file is 
opened/truncated). This can destroy agent logs and make diagnosing backup 
issues harder. Use append redirection (and quote the path) instead.



##########
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>$logFile | cut -f1) || backup_size=0
+  
+  timeout $TIMEOUT umount "$mount_point" 2>$logFile || true
+  rmdir "$mount_point" 2>$logFile || true

Review Comment:
   Both `umount` and `rmdir` redirect stderr with `2>$logFile`, which truncates 
the CloudStack agent log on every backup attempt. Use append redirection (and 
quote variables) so cleanup errors are captured without wiping existing logs.



-- 
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]

Reply via email to