Copilot commented on code in PR #12779:
URL: https://github.com/apache/cloudstack/pull/12779#discussion_r3186884004
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -1144,78 +1132,89 @@ public boolean stopVirtualMachine(long userId, long
vmId) {
status = vmEntity.stop(Long.toString(userId));
} catch (ResourceUnavailableException e) {
logger.debug("Unable to stop due to ", e);
- status = false;
} catch (CloudException e) {
throw new CloudRuntimeException("Unable to contact the agent to
stop the Instance " + vm, e);
}
return status;
}
- private UserVm rebootVirtualMachine(long userId, long vmId, boolean
enterSetup, boolean forced) throws InsufficientCapacityException,
ResourceUnavailableException {
+ private UserVm rebootVirtualMachine(long vmId, boolean enterSetup, boolean
forced) throws InsufficientCapacityException, ResourceUnavailableException {
UserVmVO vm = _vmDao.findById(vmId);
- if (logger.isTraceEnabled()) {
- logger.trace("reboot {} with enterSetup set to {}", vm,
Boolean.toString(enterSetup));
- }
-
if (vm == null || vm.getState() == State.Destroyed || vm.getState() ==
State.Expunging || vm.getRemoved() != null) {
logger.warn("Vm {} with id={} doesn't exist or is not in correct
state", vm, vmId);
return null;
}
- if (vm.getState() == State.Running && vm.getHostId() != null) {
- collectVmDiskAndNetworkStatistics(vm, State.Running);
+ if (vm.getState() != State.Running || vm.getHostId() == null) {
+ logger.error("Vm {} is not in Running state, failed to reboot",
vm);
+ return null;
+ }
+ if ( null == vm.getHostId() ) {
Review Comment:
`rebootVirtualMachine` checks `vm.getHostId()` for null twice. Because the
first condition already returns when hostId is null, the second check becomes
dead code and its more specific log message will never be emitted. Consider
removing the duplicate check (or splitting the first condition into separate
branches with accurate messages).
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -4644,10 +4588,9 @@ private UserVm getUncheckedUserVmResource(DataCenter
zone, String hostName, Stri
}
}
- if (hostName != null) {
- // Check is hostName is RFC compliant
- checkNameForRFCCompliance(hostName);
- }
+ // Check is hostName is RFC compliant
+ checkNameForRFCCompliance(hostName);
+
Review Comment:
`checkNameForRFCCompliance(hostName)` is now called unconditionally after
the hostname is finalized, but there is still an earlier conditional validation
when `hostName != null`. For requests that provide a hostname, this validates
the same hostname twice. Consider keeping only the post-normalization
validation (or removing the earlier one) to avoid duplicate work and simplify
the control flow.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -4314,6 +4242,25 @@ private UserVm createVirtualMachine(DataCenter zone,
ServiceOffering serviceOffe
return vm;
}
+ @Nullable
+ private static HypervisorType
getHypervisorTypeFromHypervisorAndTemplate(HypervisorType hypervisor,
VMTemplateVO template) {
+ HypervisorType hypervisorType = null;
+ if (template != null) {
+ if (template.getHypervisorType() == null ||
template.getHypervisorType() == HypervisorType.None) {
+ if (hypervisor == null || hypervisor == HypervisorType.None) {
+ throw new InvalidParameterValueException("Hypervisor
parameter is needed to deploy VM or the hypervisor parameter value passed is
invalid");
+ }
+ hypervisorType = hypervisor;
+ } else {
+ if (hypervisor != null && hypervisor != HypervisorType.None &&
hypervisor != template.getHypervisorType()) {
+ throw new InvalidParameterValueException("Hypervisor
passed to the deployVm call, is different from the hypervisor type of the
Template");
+ }
+ hypervisorType = template.getHypervisorType();
+ }
+ }
+ return hypervisorType;
Review Comment:
`getHypervisorTypeFromHypervisorAndTemplate` initializes `hypervisorType` to
null and only assigns it when `template != null`. If `template` is unexpectedly
null, this method returns null but the caller later dereferences
`hypervisorType`, which can lead to an NPE. Consider failing fast when
`template` is null and making this helper return a non-null `HypervisorType`.
--
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]
