Gilad Chaplik 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? yes, external vm doesn't have disks. 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? Done 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 anywa Done 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 Done 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 in all other places that's the way we add roles (don't know why), prefer to leave it if you don't mind :-) 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
