Gilad Chaplik has posted comments on this change.

Change subject: core: add disk profile to disk's commands
......................................................................


Patch Set 19:

(3 comments)

http://gerrit.ovirt.org/#/c/29038/19/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java:

Line 527:             DiskImage diskImage = (DiskImage) getNewDisk();
Line 528:             Map<DiskImage, Guid> map = new HashMap<>();
Line 529:             map.put(diskImage, diskImage.getStorageIds().get(0));
Line 530:             return 
validate(DiskProfileHelper.setAndValidateDiskProfiles(map,
Line 531:                     getStoragePool().getcompatibility_version()));
> did you verify that getStoragePool() doesn't return null?
yes
Line 532:         }
Line 533:         return true;
Line 534:     }
Line 535: 


http://gerrit.ovirt.org/#/c/29038/19/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java:

Line 261:     protected boolean setAndValidateDiskProfiles() {
Line 262:         Map<DiskImage, Guid> map = new HashMap<>();
Line 263:         for (LiveMigrateDiskParameters parameters : 
getParameters().getParametersList()) {
Line 264:             DiskImage diskImage = 
getDiskImageByImageId(parameters.getImageId());
Line 265:             if (diskImage != null && diskImage.getDiskStorageType() 
== DiskStorageType.IMAGE) {
> you already know that it is IMAGE, don't you?
Done
Line 266:                 map.put(diskImage, diskImage.getStorageIds().get(0));
Line 267:             }
Line 268:         }
Line 269:         return 
validate(DiskProfileHelper.setAndValidateDiskProfiles(map,


Line 316:                 return false;
Line 317:             }
Line 318:         }
Line 319: 
Line 320:         if (!setAndValidateDiskProfiles()) {
> why not put it in the canDoAction method?
it is in the can do...
Line 321:             return false;
Line 322:         }
Line 323: 
Line 324:         return true;


-- 
To view, visit http://gerrit.ovirt.org/29038
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7b6d977243cffc0bde772665ebaca47340075c6
Gerrit-PatchSet: 19
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
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

Reply via email to