Liron Aravot has posted comments on this change. Change subject: core: ensure unique unit for VirtIO_SCSI and SPAR_VSCSI disks ......................................................................
Patch Set 6: Code-Review+2 (2 comments) https://gerrit.ovirt.org/#/c/39471/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java: Line 351: * @param vmDevice Line 352: * @param diskInterface Line 353: * @return disk's address map Line 354: */ Line 355: public Map<String, String> getDiskAddressMap(VmDevice vmDevice, DiskInterface diskInterface) { it's not being reflected anywhere (not even in the javadoc) that this method might also update the db with the new address - which i find bit confusing, if you agree please consider to reflect that somehow Line 356: String address = vmDevice.getAddress(); Line 357: if (diskInterface != DiskInterface.VirtIO_SCSI && diskInterface != DiskInterface.SPAPR_VSCSI) { Line 358: if (StringUtils.isNotBlank(address)) { Line 359: return XmlRpcStringUtils.string2Map(address); Line 409: Line 410: return addressMap; Line 411: } Line 412: Line 413: protected void updateVmDeviceAddress(final String address, final VmDevice vmDevice) { 1. if this code is executed within a transaction, we might have an issue here as this changes aren't committed immediately, so another caller might use the same address (unfurtonatly that's relevant for AttachDiskToVmCommand) after the lock is released, we should change that command to be non transactive (not in this patch). 2. Compensation isn't used here which might be problematic - let's try this scenario: a. i want to plug disk A with SPAPR_VSCSI interface, i fail - the device remains with the selected address here (because compensation isn't used). 2. I change the disk interface to IDE and attempt to plug it. 3. the old address for SPAPR_VSCSI will be passed again along the IDE interface which will cause it fail. Line 414: vmDevice.setAddress(address); Line 415: vmDeviceDao.update(vmDevice); Line 416: } Line 417: -- To view, visit https://gerrit.ovirt.org/39471 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5e621618e8891fca52b48bff437cc4c2a1f695db Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Vitor de Lima <vdel...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
