Moti Asayag has posted comments on this change. Change subject: core: ensure unique unit for VirtIO_SCSI and SPAR_VSCSI disks ......................................................................
Patch Set 3: Code-Review-1 (2 comments) https://gerrit.ovirt.org/#/c/39471/3/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugDiskVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugDiskVDSCommand.java: Line 111: return drive; Line 112: } Line 113: Line 114: private void addAddress(Map<String, Object> map, String address) { Line 115: lockVmWithWait(); general comment: locking should be handle in try-finally block to avoid orphaned locks. lock(); try { ... } finally { unlock(); } Line 116: if (getParameters().getDisk().getDiskInterface() != DiskInterface.VirtIO_SCSI && Line 117: getParameters().getDisk().getDiskInterface() != DiskInterface.SPAPR_VSCSI) { Line 118: if (StringUtils.isNotBlank(address)) { Line 119: map.put("address", XmlRpcStringUtils.string2Map(address)); Line 180: }); Line 181: } Line 182: Line 183: protected void lockVmWithWait() { Line 184: Map<String, Pair<String, String>> exsluciveLock = > that's a clear violation of the usage of engine lock mech. its for the prov +1, serializing/controlling the action should be on done bll level. I'd move one step further and suggest not to perform any db action from this class/layer. The bll command should orchestrate bll logic, handle db, invoke the vdsm call (which basically should be the only responsibility of this class), and based on its result, decide how to proceed. Line 185: Collections.singletonMap(getParameters().getVmId().toString(), Line 186: new Pair<>(LockingGroup.VM_DISK_HOT_PLUG.toString(), Line 187: VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED.toString())); Line 188: vmLock = new EngineLock(exsluciveLock, null); -- 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: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Vitor de Lima <[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
