Arik Hadas has posted comments on this change. Change subject: engine: back-compat for disk profiles ......................................................................
Patch Set 2: (5 comments) http://gerrit.ovirt.org/#/c/36796/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 649: for (DiskImage diskImage : diskInfoDestinationMap.values()) { Line 650: map.put(diskImage, diskImage.getStorageIds().get(0)); Line 651: } Line 652: return validate(DiskProfileHelper.setAndValidateDiskProfiles(map, Line 653: getStoragePool().getcompatibility_version(), getCurrentUser())); did you verify it when VM is added "automatically" by the monitoring? Line 654: } Line 655: return true; Line 656: } Line 657: http://gerrit.ovirt.org/#/c/36796/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileHelper.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileHelper.java: Line 40: return ValidationResult.VALID; Line 41: } Line 42: Line 43: Map<Guid, List<DiskProfile>> storageDiskProfilesMap = new HashMap<>(); Line 44: Set<Guid> permittedDiskProfilesIds = new HashSet<>(); how about adding a comment that it is used only for caching? Line 45: for (Entry<DiskImage, Guid> entry : map.entrySet()) { Line 46: DiskImage diskImage = entry.getKey(); Line 47: Guid storageDomainId = entry.getValue(); Line 48: if (diskImage.getDiskProfileId() == null && storageDomainId != null) { Line 79: Set<Guid> permittedDiskProfilesIds, Line 80: DbUser user) { Line 81: if (diskProfilesList.isEmpty()) { Line 82: return false; Line 83: } I would drop this check, the compiler probably does this optimization anyway.. Line 84: for (DiskProfile diskProfile : diskProfilesList) { Line 85: if (diskProfilePermitted(diskProfile, permittedDiskProfilesIds, user)) { Line 86: permittedDiskProfilesIds.add(diskProfile.getId()); Line 87: diskImage.setDiskProfileId(diskProfile.getId()); Line 90: } Line 91: return false; Line 92: } Line 93: Line 94: private static boolean diskProfilePermitted(DiskProfile diskProfile, Set<Guid> permittedDiskProfilesIds, DbUser user) { s\diskProfilePermitted\isDiskProfilePermitted Line 95: return permittedDiskProfilesIds.contains(diskProfile.getId()) Line 96: || getPermissionDAO().getEntityPermissions(user.getId(), Line 97: ActionGroup.ATTACH_DISK_PROFILE, Line 98: diskProfile.getId(), http://gerrit.ovirt.org/#/c/36796/2/packaging/dbscripts/upgrade/03_06_0750_attach_disk_profile_permission.sql File packaging/dbscripts/upgrade/03_06_0750_attach_disk_profile_permission.sql: Line 15: AND name='DiskProfileUser' Line 16: AND description='Disk Profile User' Line 17: AND is_readonly=true Line 18: AND role_type=2 Line 19: AND app_mode=1); why do you need that 'where' clause? it will not always return 'true' since it doesn't exist? Line 20: Line 21: -- Add 'Attach disk profile' action group to roles: Line 22: -- newly created disk profile user Line 23: PERFORM fn_db_add_action_group_to_role(v_DISK_PROFILE_USER_ID, 1563); -- To view, visit http://gerrit.ovirt.org/36796 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie1910a086e46cbbf8eb2e40a6e6f185a2c5fa3aa Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@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
