Liron Ar has posted comments on this change. Change subject: core: validate disks existence on create snapshot ......................................................................
Patch Set 2: (4 comments) http://gerrit.ovirt.org/#/c/23706/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java: Line 505: validate(vmValidator.vmNotSavingRestoring()); Line 506: } Line 507: Line 508: private boolean isSpecifiedDisksExist(List<DiskImage> disks) { Line 509: if (disks != null) { if the list is empty it's the same, same comment about the indentation here though it's matter of preference..possibly we can just avoid enter this method if no disks were passed. Line 510: List<DiskImage> disksNotExistInDb = ImagesHandler.imagesSubtract(getParameters().getDisks(), getDisksList()); Line 511: if (!disksNotExistInDb.isEmpty()) { Line 512: List<String> disksNotExistInDbIds = new ArrayList<>(disksNotExistInDb.size()); Line 513: for (DiskImage diskImage : disksNotExistInDb) { Line 507: Line 508: private boolean isSpecifiedDisksExist(List<DiskImage> disks) { Line 509: if (disks != null) { Line 510: List<DiskImage> disksNotExistInDb = ImagesHandler.imagesSubtract(getParameters().getDisks(), getDisksList()); Line 511: if (!disksNotExistInDb.isEmpty()) { perhaps change to if (disksNotExistInDb.isEmpty()) { return true; } as the lines are bit long here, it'll take the whole code an indentation left. Line 512: List<String> disksNotExistInDbIds = new ArrayList<>(disksNotExistInDb.size()); Line 513: for (DiskImage diskImage : disksNotExistInDb) { Line 514: disksNotExistInDbIds.add(diskImage.getId().toString()); Line 515: } Line 508: private boolean isSpecifiedDisksExist(List<DiskImage> disks) { Line 509: if (disks != null) { Line 510: List<DiskImage> disksNotExistInDb = ImagesHandler.imagesSubtract(getParameters().getDisks(), getDisksList()); Line 511: if (!disksNotExistInDb.isEmpty()) { Line 512: List<String> disksNotExistInDbIds = new ArrayList<>(disksNotExistInDb.size()); there's no need to specify the size here, as the capacity grows autmatically..i usually prefer to use linkedlist in that usecase. Line 513: for (DiskImage diskImage : disksNotExistInDb) { Line 514: disksNotExistInDbIds.add(diskImage.getId().toString()); Line 515: } Line 516: Line 514: disksNotExistInDbIds.add(diskImage.getId().toString()); Line 515: } Line 516: Line 517: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_NOT_EXIST, Line 518: String.format("$%1$s %2$s", "diskIds", StringUtils.join(disksNotExistInDbIds, ","))); perhaps we can have this check in DiskValidator so we could re-use it. Line 519: } Line 520: } Line 521: Line 522: return true; -- To view, visit http://gerrit.ovirt.org/23706 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d75ad139d048f83b08d9289e43d909b29f89695 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[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
