Ayal Baron has posted comments on this change.
Change subject: core: Extract SD validations from ImagesHandler
......................................................................
Patch Set 7: (4 inline comments)
Just a partial review
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java
Line 27: import
org.ovirt.engine.core.common.businessentities.Disk.DiskStorageType;
Line 28: import org.ovirt.engine.core.common.businessentities.DiskImage;
Line 29: import org.ovirt.engine.core.common.businessentities.Snapshot;
Line 30: import
org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotType;
Line 31: import org.ovirt.engine.core.common.businessentities.StorageDomain;
this is a big patch already and all these style changes are just annoying in
this context. Personally I'd prefer to see these changes as the first of 2
patches in a set and then be able to focus only on the real changes.
Line 32: import org.ovirt.engine.core.common.businessentities.StorageDomainType;
Line 33: import
org.ovirt.engine.core.common.businessentities.StoragePoolIsoMapId;
Line 34: import org.ovirt.engine.core.common.businessentities.VM;
Line 35: import org.ovirt.engine.core.common.businessentities.VmTemplate;
Line 173:
Line 174: return true;
Line 175: }
Line 176:
Line 177: @Override
same as above (unless this is actually solving something in which case it
totally doesn't belong in the patch set even)
Line 178: protected boolean
doesStorageDomainhaveSpaceForRequest(StorageDomain storageDomain, long
sizeRequested) {
Line 179: return validate(new
StorageDomainValidator(storageDomain).isDomainHasSpaceForRequest(sizeRequested));
Line 180: }
Line 181:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
Line 363: String fileName = Path.GetFileName(windowsPath);
Line 364: return String.format("%1$s/%2$s", isoPrefix, fileName);
Line 365: }
Line 366:
Line 367: public static boolean isImagesExists(Iterable<DiskImage> images,
Guid storagePoolId) {
The name "isImagesExists" is wrong in so many ways :)
doesImageExist?
doImagesExist?
Line 368: return isImagesExists(images, storagePoolId, new
ArrayList<DiskImage>());
Line 369: }
Line 370:
Line 371: private static boolean isImagesExists(Iterable<DiskImage> images,
Guid storagePoolId, ArrayList<DiskImage> irsImages) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java
Line 230: if (!listVms.isEmpty()) {
Line 231: Guid storagePoolId = listVms.get(0).getStoragePoolId();
Line 232: storage_pool sp = getStoragePoolDAO().get(storagePoolId);
Line 233: if (!validate(new StoragePoolValidator(sp).isUp())) {
Line 234: return false;
moving these checks out of the loop is a very welcome change indeed, but again,
has nothing to do with getting rid of PerformImagesChecks in general? and make
reading this patch more difficult (as here there is a change that merits
reviewing properly in and by itself).
Line 235: }
Line 236:
Line 237: if (!ImagesHandler.PerformImagesChecks(
Line 238: getReturnValue().getCanDoActionMessages(),
--
To view, visit http://gerrit.ovirt.org/12249
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cfc895dceed4de520476ed5e3fa9c1c7cfd78f4
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
