Copilot commented on code in PR #13263:
URL: https://github.com/apache/cloudstack/pull/13263#discussion_r3310970947
##########
server/src/main/java/org/apache/cloudstack/vm/ImportVmTasksManagerImpl.java:
##########
@@ -161,6 +161,9 @@ public void updateImportVMTaskStep(ImportVmTask
importVMTask, DataCenter zone, A
@Override
public void updateImportVMTaskErrorState(ImportVmTask importVMTask,
ImportVmTask.TaskState state, String errorMsg) {
+ if (importVMTask == null) {
Review Comment:
`updateImportVMTaskErrorState` silently returns when `importVMTask` is null.
That can make it hard to diagnose why the import task row wasn’t updated (e.g.,
task creation failed earlier). Consider logging at least a WARN/DEBUG
(including `state` and `errorMsg`) before returning so the original failure
remains observable in server logs.
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -1777,7 +1777,9 @@ protected UserVm
importUnmanagedInstanceFromVmwareToKvm(DataCenter zone, Cluster
return userVm;
} catch (CloudRuntimeException e) {
logger.error(String.format("Error importing VM: %s",
e.getMessage()), e);
- importVmTasksManager.updateImportVMTaskErrorState(importVMTask,
ImportVmTask.TaskState.Failed, e.getMessage());
+ if (importVMTask != null) {
+
importVmTasksManager.updateImportVMTaskErrorState(importVMTask,
ImportVmTask.TaskState.Failed, e.getMessage());
+ }
Review Comment:
`updateImportVMTaskErrorState(...)` now performs its own null guard, so this
additional null check in the caller is redundant and duplicates the contract in
two places. Consider removing the caller-side guard and relying on the manager
method’s null handling to keep the behavior centralized and avoid future
divergence.
--
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]
