Maor Lipchuk has posted comments on this change.
Change subject: core: fail deletion of template disk with derived disks
......................................................................
Patch Set 2:
(4 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java
Line 200: private DiskImagesValidator createDiskImagesValidator(DiskImage
disk) {
Line 201: return new DiskImagesValidator(Arrays.asList(disk));
Line 202: }
Line 203:
Line 204: private boolean checkDerivedDisksFromDiskNotExist(DiskImage
diskImage) {
I would recommend to change the method name (The FromDiskNotExist sounds
strange to me, and also this method is specifically for template), how about
isTemplateDiskHasNoDerivedDisks?
Line 205: return
validate(createDiskImagesValidator(diskImage).diskImagesHaveNoDerivedDisks(getParameters().getStorageDomainId()));
Line 206: }
Line 207:
Line 208: private List<String> getNamesOfDerivedVmsFromTemplate(DiskImage
diskImage) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/DiskImagesValidator.java
Line 129: return ValidationResult.VALID;
Line 130: }
Line 131:
Line 132:
Line 133: public ValidationResult diskImagesHaveNoDerivedDisks(Guid
storageDomainId) {
a null storageDomainId indicating it refers to any disks on any storages should
be referenced in a javadoc IMHO.
Line 134: List<String> disksInfo = null;
Line 135: for (DiskImage diskImage : diskImages) {
Line 136: if (diskImage.getVmEntityType() != null &&
diskImage.getVmEntityType().isTemplateType()) {
Line 137: List<DiskImage> basedDisks =
getDiskImageDAO().getAllSnapshotsForParent(diskImage.getImageId());
Line 130: }
Line 131:
Line 132:
Line 133: public ValidationResult diskImagesHaveNoDerivedDisks(Guid
storageDomainId) {
Line 134: List<String> disksInfo = null;
You can simply declare new LinkedList<> instead of initializing this in the
middle of the for loop (at line 141), and just check if this is not empty at
line 149.
Line 135: for (DiskImage diskImage : diskImages) {
Line 136: if (diskImage.getVmEntityType() != null &&
diskImage.getVmEntityType().isTemplateType()) {
Line 137: List<DiskImage> basedDisks =
getDiskImageDAO().getAllSnapshotsForParent(diskImage.getImageId());
Line 138: for (DiskImage basedDisk : basedDisks) {
Line 132:
Line 133: public ValidationResult diskImagesHaveNoDerivedDisks(Guid
storageDomainId) {
Line 134: List<String> disksInfo = null;
Line 135: for (DiskImage diskImage : diskImages) {
Line 136: if (diskImage.getVmEntityType() != null &&
diskImage.getVmEntityType().isTemplateType()) {
1) If VmEntityType is null then this is a bug and we should not return true in
this validation.
2) Why should we validate this condition for each disk in the template, this
list of disks should be related to only one entity (A template for now)
Line 137: List<DiskImage> basedDisks =
getDiskImageDAO().getAllSnapshotsForParent(diskImage.getImageId());
Line 138: for (DiskImage basedDisk : basedDisks) {
Line 139: if (storageDomainId == null ||
basedDisk.getStorageIds().contains(storageDomainId)) {
Line 140: if (disksInfo == null) {
--
To view, visit http://gerrit.ovirt.org/22428
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifca48993f40f3c5f7b603f33add7049e78e6c4e9
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Tal Nisan <[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
