Ayal Baron has posted comments on this change. Change subject: engine: Illegal images should be skipped when validating duplicates ......................................................................
Patch Set 5: (2 comments) .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java Line 638: public static List<DiskImage> getDisksFromDB(List<DiskImage> disks) { Line 639: List<DiskImage> disksInDB = new ArrayList<>(); Line 640: for (DiskImage disk : disks) { Line 641: DiskImage diskImage = DbFacade.getInstance().getDiskImageDao().get(disk.getId()); Line 642: if (diskImage != null) { how and why would diskImage be null? why is it ok to pass such things around? Line 643: disksInDB.add(diskImage); Line 644: } Line 645: } Line 646: return disksInDB; .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java Line 337: if (!validateNoDuplicateVm()) { Line 338: return false; Line 339: } Line 340: Line 341: if (!getParameters().isImportAsNewEntity() && !validateNoDuplicateDiskImages(imageList)) { It looks to me like testing if isImportAsNewEntity under 'canDoActionAfter*Clone*Vm' is redundant? (since obviously we are, unless this method is really badly named) and validateNoDup... is plain wrong in this scenario. Line 342: return false; Line 343: } Line 344: Line 345: if (!validateDiskInterface(imageList)) { -- To view, visit http://gerrit.ovirt.org/19246 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7b43e4b4e8e04dfb644a33ea9ce2a474987c5e3c Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> 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