Omer Frenkel has posted comments on this change. Change subject: backend: Add HostDev passthrough support #3 ......................................................................
Patch Set 31: (4 comments) look ok minor comments https://gerrit.ovirt.org/#/c/37619/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmHostDeviceByVmIdAndDeviceNameQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmHostDeviceByVmIdAndDeviceNameQuery.java: Line 19: } Line 20: Line 21: @Override Line 22: protected void executeQueryCommand() { Line 23: List<VmDevice> vmDevices = vmDeviceDAO.getVmDeviceByVmIdTypeAndDevice(getParameters().getId(), if this is a user query (as defined in the queryType) you should use the userId and filtered params Line 24: VmDeviceGeneralType.HOSTDEV, getParameters().getDeviceName()); Line 25: if (vmDevices != null && !vmDevices.isEmpty()) { Line 26: setReturnValue(new VmHostDevice(vmDevices.get(0))); Line 27: } https://gerrit.ovirt.org/#/c/37619/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmHostDevicesQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmHostDevicesQuery.java: Line 27: @Override Line 28: protected void executeQueryCommand() { Line 29: List<VmHostDevice> result = new ArrayList<>(); Line 30: List<VmDevice> vmHostDevices = vmDeviceDao.getVmDeviceByVmIdAndType( Line 31: getParameters().getId(), VmDeviceGeneralType.HOSTDEV); same comment for user query Line 32: Line 33: for (VmDevice device : vmHostDevices) { Line 34: result.add(new VmHostDevice(device)); Line 35: } https://gerrit.ovirt.org/#/c/37619/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/AbstractVmHostDevicesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/AbstractVmHostDevicesCommand.java: Line 67: public List<PermissionSubject> getPermissionCheckSubjects() { Line 68: List<PermissionSubject> permissionList = new ArrayList<>(); Line 69: // TODO: validate host permission, since running VM with pass-through devices can have inadvertent harmful Line 70: // effects to the host. Maybe this permission should be checked in UpdateVmCommand when the VM is being Line 71: // pinned to another host? on updateVm, there is an extra requirement for EDIT_ADMIN_VM_PROPERTIES when setting pinning, it should be enough, by default the following roles has it: SuperUser, ClusterAdmin, DataCenterAdmin but please add this info the the feature page and also in the bug doc_text Line 72: permissionList.addAll(super.getPermissionCheckSubjects()); Line 73: return permissionList; Line 74: } Line 75: https://gerrit.ovirt.org/#/c/37619/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/AddVmHostDevicesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/AddVmHostDevicesCommand.java: Line 37: namesAdded.add(hostDevice.getDeviceName()); Line 38: } Line 39: } Line 40: return devicesToAdd; Line 41: } what about audit logging? Line 42: } -- To view, visit https://gerrit.ovirt.org/37619 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93c746cdda71678f7840d37683b890080a74341d Gerrit-PatchSet: 31 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Betak <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Martin Betak <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Shahar Havivi <[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
