Liron Ar has posted comments on this change. Change subject: engine: Check for conflicting address when hotplugging a disk ......................................................................
Patch Set 7: (2 comments) http://gerrit.ovirt.org/#/c/26598/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotPlugDiskToVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotPlugDiskToVmCommand.java: Line 146: * Clear the device address if a device is being hot plugged and its stored address is already in use by another Line 147: * plugged device Line 148: **/ Line 149: private void clearAddressAlreadyInUse() { Line 150: if (!oldVmDevice.getIsPlugged()) { 1. if the address is of the device is already "" we should early return as well. 2. the if clause should be the opposite isn't it? the device is unplugged at this stage. Line 151: return; Line 152: } Line 153: List<VmDevice> devices = getVmDeviceDao().getVmDeviceByVmIdAndType(getVmId(), oldVmDevice.getType()); Line 154: for (VmDevice device : devices) { Line 149: private void clearAddressAlreadyInUse() { Line 150: if (!oldVmDevice.getIsPlugged()) { Line 151: return; Line 152: } Line 153: List<VmDevice> devices = getVmDeviceDao().getVmDeviceByVmIdAndType(getVmId(), oldVmDevice.getType()); can't the same address be used for different device types (disk and cdrom for example)? perhaps it'll be better to add a dedicated "getByVmIdAndAddress(AndPlugStatus)" method to VmDeviceDao Line 154: for (VmDevice device : devices) { Line 155: if (!device.getId().equals(oldVmDevice.getId()) && device.getIsPlugged() && oldVmDevice.getAddress().equals(device.getAddress())) { Line 156: oldVmDevice.setAddress(""); Line 157: getVmDeviceDao().clearDeviceAddress(oldVmDevice.getDeviceId()); -- To view, visit http://gerrit.ovirt.org/26598 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id5850fd4c15a230dbc27e8c22d47935083da21c2 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Xavi Francisco <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Xavi Francisco <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
