Copilot commented on code in PR #12349:
URL: https://github.com/apache/cloudstack/pull/12349#discussion_r2652177051
##########
server/src/main/java/com/cloud/network/guru/PrivateNetworkGuru.java:
##########
@@ -189,8 +189,12 @@ protected void getIp(NicProfile nic, DataCenter dc,
Network network) throws Insu
PrivateIpVO ipVO =
_privateIpDao.allocateIpAddress(network.getDataCenterId(), network.getId(),
null);
String vlanTag =
BroadcastDomainType.getValue(network.getBroadcastUri());
String netmask = NetUtils.getCidrNetmask(network.getCidr());
+ String macAddress =
NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(ipVO.getMacAddress(),
networkModel.getMacIdentifier(network.getDataCenterId())));
+ if (!networkModel.isMACUnique(macAddress, network.getId())) {
+ macAddress =
networkModel.getNextAvailableMacAddressInNetwork(network.getId());
Review Comment:
Extra space after the equals operator. This should be a single space for
consistency with Java coding conventions.
```suggestion
macAddress =
networkModel.getNextAvailableMacAddressInNetwork(network.getId());
```
##########
server/src/main/java/com/cloud/network/router/NicProfileHelperImpl.java:
##########
@@ -90,14 +91,20 @@ public NicProfile createPrivateNicProfileForGateway(final
VpcGateway privateGate
privateNicProfile.setDeviceId(null);
if (router.getIsRedundantRouter()) {
- String newMacAddress =
NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(ipVO.getMacAddress(),
NetworkModel.MACIdentifier.value()));
+ String newMacAddress =
NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(ipVO.getMacAddress(),
_networkModel.getMacIdentifier(privateNetwork.getDataCenterId())));
+ if (!_networkModel.isMACUnique(newMacAddress,
privateNetwork.getId())) {
+ newMacAddress =
_networkModel.getNextAvailableMacAddressInNetwork(privateNetwork.getId());
+ }
privateNicProfile.setMacAddress(newMacAddress);
}
} else {
final String netmask =
NetUtils.getCidrNetmask(privateNetwork.getCidr());
+ String newMacAddress =
NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(ipVO.getMacAddress(),
_networkModel.getMacIdentifier(privateNetwork.getDataCenterId())));
+ if (!_networkModel.isMACUnique(newMacAddress,
privateNetwork.getId())) {
+ newMacAddress =
_networkModel.getNextAvailableMacAddressInNetwork(privateNetwork.getId());
Review Comment:
Extra space after the equals operator. This should be a single space for
consistency with Java coding conventions.
##########
server/src/main/java/com/cloud/network/guru/StorageNetworkGuru.java:
##########
@@ -130,7 +132,11 @@ public void reserve(NicProfile nic, Network network,
VirtualMachineProfile vm, D
vlan = ip.getVlan();
nic.setIPv4Address(ip.getIpAddress());
-
nic.setMacAddress(NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(ip.getMac(),
NetworkModel.MACIdentifier.value())));
+ String macAddress =
NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(ip.getMac(),
_networkModel.getMacIdentifier(dest.getDataCenter().getId())));
+ if (!_networkModel.isMACUnique(macAddress, network.getId())) {
+ macAddress =
_networkModel.getNextAvailableMacAddressInNetwork(network.getId());
Review Comment:
Extra space after the equals operator. This should be a single space for
consistency with Java coding conventions.
```suggestion
macAddress =
_networkModel.getNextAvailableMacAddressInNetwork(network.getId());
```
##########
utils/src/main/java/com/cloud/utils/net/NetUtils.java:
##########
@@ -131,17 +131,17 @@ public static boolean
isIpv6EnabledProtocol(InternetProtocol protocol) {
}
}
- public static long createSequenceBasedMacAddress(final long macAddress,
long globalConfig) {
+ public static long createSequenceBasedMacAddress(final long macAddress,
long macIdentifier) {
/*
Logic for generating MAC address:
Mac = B1:B2:B3:B4:B5:B6 (Bx is a byte).
B1 -> Presently controlled by prefix variable. The value should be
such that the MAC is local and unicast.
- B2 -> This will be configurable for each deployment/installation.
Controlled by the global config MACIdentifier
+ B2 -> This will be configurable for each deployment/installation.
Controlled by the 'mac.identifier' zone-level config
B3 -> A randomly generated number between 0 - 255
B4,5,6 -> These bytes are based on the unique DB identifier
associated with the IP address for which MAC is generated (refer to mac_address
field in user_ip_address table).
*/
- return macAddress | prefix<<40 | globalConfig << 32 & 0x00ff00000000l
| (long)s_rand.nextInt(255) << 24;
+ return macAddress | prefix << 40 | macIdentifier << 32 &
0x00ff00000000L | (long)s_rand.nextInt(255) << 24;
Review Comment:
The random number generation uses `s_rand.nextInt(255)` which generates
values from 0 to 254 (exclusive upper bound). According to the comment on line
140, B3 should be "A randomly generated number between 0 - 255". To include
255, this should be `s_rand.nextInt(256)`.
```suggestion
return macAddress | prefix << 40 | macIdentifier << 32 &
0x00ff00000000L | (long)s_rand.nextInt(256) << 24;
```
##########
server/src/main/java/com/cloud/network/guru/PodBasedNetworkGuru.java:
##########
@@ -131,7 +133,11 @@ public void reserve(NicProfile nic, Network config,
VirtualMachineProfile vm, De
Integer vlan = result.getVlan();
nic.setIPv4Address(result.getIpAddress());
-
nic.setMacAddress(NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(result.getMacAddress(),
NetworkModel.MACIdentifier.value())));
+ String macAddress =
NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(result.getMacAddress(),
_networkModel.getMacIdentifier(dest.getDataCenter().getId())));
+ if (!_networkModel.isMACUnique(macAddress, config.getId())) {
+ macAddress =
_networkModel.getNextAvailableMacAddressInNetwork(config.getId());
Review Comment:
Extra space after the equals operator. This should be a single space for
consistency with Java coding conventions.
```suggestion
macAddress =
_networkModel.getNextAvailableMacAddressInNetwork(config.getId());
```
##########
engine/schema/src/main/java/com/cloud/vm/dao/NicDaoImpl.java:
##########
@@ -400,9 +400,10 @@ public List<NicVO> listByVmIdAndKeyword(long instanceId,
String keyword) {
}
@Override
- public NicVO findByMacAddress(String macAddress) {
+ public NicVO findByMacAddress(String macAddress, long networkId) {
SearchCriteria<NicVO> sc = AllFieldsSearch.create();
sc.setParameters("macAddress", macAddress);
+ sc.setParameters("networkId", networkId);
Review Comment:
The parameter name should be "network" not "networkId" to match the
AllFieldsSearch builder definition on line 63 of the init() method. Using
"networkId" will cause the network filter to be silently ignored, effectively
reverting to a global MAC address uniqueness check instead of network-scoped
uniqueness as intended by this PR.
```suggestion
sc.setParameters("network", networkId);
```
##########
server/src/main/java/com/cloud/network/router/NicProfileHelperImpl.java:
##########
@@ -90,14 +91,20 @@ public NicProfile createPrivateNicProfileForGateway(final
VpcGateway privateGate
privateNicProfile.setDeviceId(null);
if (router.getIsRedundantRouter()) {
- String newMacAddress =
NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(ipVO.getMacAddress(),
NetworkModel.MACIdentifier.value()));
+ String newMacAddress =
NetUtils.long2Mac(NetUtils.createSequenceBasedMacAddress(ipVO.getMacAddress(),
_networkModel.getMacIdentifier(privateNetwork.getDataCenterId())));
+ if (!_networkModel.isMACUnique(newMacAddress,
privateNetwork.getId())) {
+ newMacAddress =
_networkModel.getNextAvailableMacAddressInNetwork(privateNetwork.getId());
Review Comment:
Extra space after the equals operator. This should be a single space for
consistency with Java coding conventions.
##########
server/src/main/java/com/cloud/network/NetworkModelImpl.java:
##########
@@ -593,22 +593,23 @@ public List<? extends Nic> getNics(long vmId) {
@Override
public String getNextAvailableMacAddressInNetwork(long networkId) throws
InsufficientAddressCapacityException {
NetworkVO network = _networksDao.findById(networkId);
- Integer zoneIdentifier = MACIdentifier.value();
- if (zoneIdentifier.intValue() == 0) {
- zoneIdentifier =
Long.valueOf(network.getDataCenterId()).intValue();
+ if (network == null) {
+ throw new CloudRuntimeException("Could not find network with id "
+ networkId);
}
+
+ Integer zoneMacIdentifier =
Long.valueOf(getMacIdentifier(network.getDataCenterId())).intValue();
String mac;
do {
- mac = _networksDao.getNextAvailableMacAddress(networkId,
zoneIdentifier);
+ mac = _networksDao.getNextAvailableMacAddress(networkId,
zoneMacIdentifier);
if (mac == null) {
throw new InsufficientAddressCapacityException("Unable to
create another mac address", Network.class, networkId);
}
- } while(! isMACUnique(mac));
+ } while(! isMACUnique(mac, networkId));
Review Comment:
There should be a space after 'while' and before the opening parenthesis.
The '!' operator should also not have a space after it for consistency with
Java conventions.
```suggestion
} while (!isMACUnique(mac, networkId));
```
##########
engine/schema/src/main/java/com/cloud/network/dao/NetworkDaoImpl.java:
##########
@@ -432,8 +432,8 @@ public List<NetworkVO>
getAllPersistentNetworksFromZone(long dataCenterId) {
public String getNextAvailableMacAddress(final long networkConfigId,
Integer zoneMacIdentifier) {
final SequenceFetcher fetch = SequenceFetcher.getInstance();
long seq = fetch.getNextSequence(Long.class, _tgMacAddress,
networkConfigId);
- if(zoneMacIdentifier != null && zoneMacIdentifier.intValue() != 0 ){
- seq = seq | _prefix << 40 | (long)zoneMacIdentifier << 32 |
networkConfigId << 16 & 0x00000000ffff0000l;
+ if (zoneMacIdentifier != null && zoneMacIdentifier != 0 ) {
Review Comment:
There is an extra space before the closing parenthesis. For consistency with
the rest of the codebase, this should be removed.
--
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]
