Copilot commented on code in PR #12781:
URL: https://github.com/apache/cloudstack/pull/12781#discussion_r2910863072
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -5405,7 +5405,12 @@ public boolean
finalizeVirtualMachineProfile(VirtualMachineProfile profile, Depl
@Override
public boolean setupVmForPvlan(boolean add, Long hostId, NicProfile nic) {
- if (!nic.getBroadCastUri().getScheme().equals("pvlan")) {
+ if (nic == null || nic.getBroadCastUri() == null) {
+ return false;
+ }
+
+ String scheme = nic.getBroadCastUri().getScheme();
+ if (!"pvlan".equals(scheme)) {
Review Comment:
Returning `false` here makes the failure mode indistinguishable from the
non-pvlan path (`scheme != pvlan`). Consider logging (at least debug/warn) why
pvlan setup is being skipped (null NIC vs null broadcast URI) so future
incidents are diagnosable.
```suggestion
if (nic == null) {
logger.debug("Skipping PVLAN setup on host {} because NIC
profile is null", hostId);
return false;
}
if (nic.getBroadCastUri() == null) {
logger.debug("Skipping PVLAN setup on host {} for NIC with MAC
{} because broadcast URI is null", hostId, nic.getMacAddress());
return false;
}
String scheme = nic.getBroadCastUri().getScheme();
if (!"pvlan".equals(scheme)) {
logger.debug("Skipping PVLAN setup on host {} for NIC with MAC
{} because broadcast URI scheme is '{}'", hostId, nic.getMacAddress(), scheme);
```
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -5415,9 +5420,14 @@ public boolean setupVmForPvlan(boolean add, Long hostId,
NicProfile nic) {
}
Network network = _networkDao.findById(nic.getNetworkId());
Host host = _hostDao.findById(hostId);
+ if (host == null) {
+ logger.warn("Host with id {} doesn't exist", hostId);
+ return false;
+ }
Review Comment:
The network lookup is performed even when `hostId` is invalid and the method
returns early. Consider fetching/validating `host` before calling
`_networkDao.findById(...)` to avoid unnecessary DAO work in this error path.
```suggestion
Host host = _hostDao.findById(hostId);
if (host == null) {
logger.warn("Host with id {} doesn't exist", hostId);
return false;
}
Network network = _networkDao.findById(nic.getNetworkId());
```
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -5415,9 +5420,14 @@ public boolean setupVmForPvlan(boolean add, Long hostId,
NicProfile nic) {
}
Network network = _networkDao.findById(nic.getNetworkId());
Host host = _hostDao.findById(hostId);
+ if (host == null) {
+ logger.warn("Host with id {} doesn't exist", hostId);
Review Comment:
Consider changing the log message to avoid the contraction \"doesn't\"
(e.g., \"does not exist\") for consistency and formality in logs.
```suggestion
logger.warn("Host with id {} does not exist", hostId);
```
--
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]
