Maor Lipchuk has posted comments on this change.
Change subject: core, restapi: Introducing support for attaching disk snapshot
......................................................................
Patch Set 26:
(4 comments)
partial reviewed - mostly cosmetic stuff
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java
Line 137: }
Line 138:
Line 139: protected void updateVmDevices() {
Line 140: VmDeviceUtils.setVmDevices(getVm().getStaticData());
Line 141: }
Please add empty line
Line 142: protected void updateVmDisks() {
Line 143: VmHandler.updateDisksFromDb(getVm());
Line 144: VmHandler.filterDisksForVM(getVm(), false, false, true);
Line 145: mImages.addAll(getVm().getDiskList());
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java
Line 339: if (!validate(storageValidator.allDomainsExistAndActive()) ||
Line 340:
!validate(storageValidator.allDomainsWithinThresholds()) ||
Line 341: !performImagesChecks() ||
Line 342: !validate(vmValidator.vmDown()) ||
Line 343: // if the user just previewed a snapshot and selected
"undo" - the vm can have snapshots attached to other vms.
I would consider to extract this to another method instead put comment in the
middle of the if condition
Line 344: (targetSnapshot.getType() != SnapshotType.PREVIEW &&
!validate(vmValidator.vmNotHavingDeviceSnapshotsAttachedToOtherVms()))) {
Line 345: return false;
Line 346: }
Line 347:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SPMAsyncTask.java
Line 371
Line 372
Line 373
Line 374
Line 375
No need to change this
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java
Line 273: boolean
allowOnlyActiveDisks) {
Line 274: List<DiskImage> filteredDisks =
ImagesHandler.filterImageDisks(vm.getDiskList(), allowOnlyNotShareableDisks,
allowOnlySnapableDisks, allowOnlyActiveDisks);
Line 275: Collection<DiskImage> vmDisksToRemove =
CollectionUtils.subtract(vm.getDiskList(), filteredDisks);
Line 276: // done so that lun disks would be included as well
Line 277: Collection<Disk> vmDisksAfterFilter =
CollectionUtils.disjunction(vm.getDiskMap().values(), vmDisksToRemove);
you use diskMap here and before you use diskList, why?
Line 278: clearVmDisks(vm);
Line 279: updateDisksForVm(vm, vmDisksAfterFilter);
Line 280: for (DiskImage diskToRemove : vmDisksToRemove) {
Line 281: vm.getManagedVmDeviceMap().remove(diskToRemove.getId());
--
To view, visit http://gerrit.ovirt.org/17679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02579bf1a91cd294a5040acf432f1fdb87eb18c1
Gerrit-PatchSet: 26
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@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: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@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
