Maor Lipchuk has posted comments on this change. Change subject: engine: Check for conflicting address when hotplugging a disk ......................................................................
Patch Set 6: (5 comments) http://gerrit.ovirt.org/#/c/26598/6/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 126: // We need to check if the assigned address for that device is already taken when we try to hotplug it, Line 127: // and in that case, clear the address. Line 128: if (!oldVmDevice.getIsPlugged()) { Line 129: clearAddressAlreadyInUse(); Line 130: } Perhaps it would be nicer if this entire code will be extended to a method, and the comment will be used as a java doc Line 131: performPlugCommand(getPlugAction(), getDisk(), oldVmDevice); Line 132: } Line 133: Line 134: // At this point disk is already plugged to or unplugged from VM (depends on the command), Line 148: Line 149: private void clearAddressAlreadyInUse() { Line 150: List<VmDevice> devices = getVmDeviceDao().getVmDeviceByVmIdAndType(getVmId(), oldVmDevice.getType()); Line 151: for (VmDevice device : devices) { Line 152: if (device.getId().equals(oldVmDevice.getId())) { You can remove that if condition and just add following to the second condition: if (!device.getId().equals(oldVmDevice.getId() && device.getIsPlugged.... Line 153: continue; Line 154: } Line 155: if (device.getIsPlugged() && oldVmDevice.getAddress().equals(device.getAddress())) { Line 156: oldVmDevice.setAddress(""); Line 152: if (device.getId().equals(oldVmDevice.getId())) { Line 153: continue; Line 154: } Line 155: if (device.getIsPlugged() && oldVmDevice.getAddress().equals(device.getAddress())) { Line 156: oldVmDevice.setAddress(""); No need to set the address with empty string, it is already being updated in the clearDeviceAddress Line 157: getVmDeviceDao().clearDeviceAddress(oldVmDevice.getDeviceId()); Line 158: break; Line 159: } Line 160: } Line 153: continue; Line 154: } Line 155: if (device.getIsPlugged() && oldVmDevice.getAddress().equals(device.getAddress())) { Line 156: oldVmDevice.setAddress(""); Line 157: getVmDeviceDao().clearDeviceAddress(oldVmDevice.getDeviceId()); There is a potential race here, since you might plug two disks with the same address, but the first one will not be active yet while the second one is checking for the same address in the active disks. I think that you can solve this by checking if there is also an unplugged device which has the same address (you can just remove the device.getIsPlugged() form the condition at line 155 Line 158: break; Line 159: } Line 160: } Line 161: } Line 159: } Line 160: } Line 161: } Line 162: Line 163: protected void updateDeviceIsPluggedProperty() { There is no need to use protected here, since this is only being used in this class Line 164: VmDevice device = getVmDeviceDao().get(oldVmDevice.getId()); Line 165: device.setIsPlugged(!oldVmDevice.getIsPlugged()); Line 166: getVmDeviceDao().updateHotPlugDisk(device); Line 167: } -- 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: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Xavi Francisco <xfran...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Xavi Francisco <xfran...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
