Ayal Baron has posted comments on this change.
Change subject: ImagesHandler PerformImageChecks refactor (WIP)
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(27 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java
Line 249: true, checkIsValid,
sourceImageDomainsImageMap.get(srcStorageDomainId))) {
should split into multiple lines for clarity
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
Line 362: private static DiskImage isImageExist(Guid storagePoolId,
s/isImageExist/getImageInfo/
Line 452: if (returnValue && checkStorageDomain) {
s/returnValue//
Line 453: if (Guid.Empty.equals(storageDomainId)) {
I believe this should be !Guid.Empty.equals
Line 457: returnValue =
either: returnValue = returnValue &&
or just return early
Line 462: if (Guid.Empty.equals(storageDomainId)) {
again, !Guid...
But these checks are redundant, just check this once regardless of the
booleans. If storageDomainId is not empty then obviously you need to check it.
Line 467:
checkDiskSpace(getStorageDomainsForImages(images, storagePoolId),
storagePoolId, messages);
again, returnValue = returnValue &&
or return early
Line 471: if (!checkImagesExist(images, storagePoolId,
storageDomainId)) {
this is inconsistent. checkImagesExist should take messages as an argument
like the rest of the methods and populate it with the below message if false.
Line 479: CheckImagesLegality(messages, images, vm,
getExistingImages(images, storagePoolId, storageDomainId));
order of parameters is inconsistent (sometimes messages is passed last and
sometimes first) need to make it consistent
Line 482: checkImagesExist(images, messages);
at first I wanted to say that you're missing "returnValue =" but then I noticed
a much more basic flaw here.
no need for "else if", just "else"
no need for "checkImagesExist" just add ACTION_TYPE_FAILED_VM_HAS_NO_DISKS
message and return false
Also, need to reverse 'if' and do this first so you can then drop the 'else'
entirely.
Line 499: if (vm.getstatus() == VMStatus.ImageIllegal) {
VM legality should not be here. looks like a misplaced optimization
Line 508:
diskImage.setimageStatus(image.getimageStatus());
why does a 'check' method *set* the image status and update the DB?
Line 512:
VdcBllMessages.ACTION_TYPE_FAILED_VM_IMAGE_IS_ILLEGAL.toString());
message should be about the disk, not about the VM.
Line 608: public static boolean checkImagesLocked(VM vm, List<String>
messages, Collection diskImageList) {
s/checkImagesLocked/checkImagesNotLocked/
Line 610: if (vm.getstatus() == VMStatus.ImageLocked) {
this checks the status of the VM, not the images so should move to a separate
check.
looks like a misplaced optimization
Line 618: returnValue = false;
return early here (and move nullSafeAdd here)
Line 625: ListUtils.nullSafeAdd(messages,
VdcBllMessages.ACTION_TYPE_FAILED_VM_IMAGE_IS_LOCKED.toString());
message is about image not about VM
Line 630: public static boolean checkVmIsDown(VM vm, List<String> messages)
{
obviously has no business in ImagesHandler
Line 638: public static boolean checkVmInPreview(VM vm, List<String>
messages) {
s/checkVmInPreview/checkVmNotInPreview/
also has no business in imagesHandler
Line 646: public static boolean checkIsValid(Guid storagePoolId,
List<String> messages) {
s/checkIsValid/checkSpmProxyUp/
Also has no business in imagesHandler
Line 657: public static boolean checkImagesExist(List<DiskImage> images,
List<String> messages) {
this doesn't check that images exist, just that image list is not empty.
As noted above, this method should be deleted
Line 673: DiskImage fromIrs = isImageExist(storagePoolId,
storageDomainId, image);
s/fromIrs/imageInfo/
Line 690: public static boolean checkStorageDomain(storage_domains
storageDomain, Guid storagePoolId, List<String> messages) {
has no business in imagesHandler
checkStorageDomainActive
Line 699: public static boolean checkStorageDomains(List<storage_domains>
storageDomains,
has no business in imagesHandler
checkStorageDomainsActive
Line 710: private static boolean checkDiskSpace(storage_domains
storageDomain, Guid storagePoolId, List<String> messages) {
has no business in imagesHandler
s/checkDiskSpace/checkStorageDomainFreeSpace/
Line 718: public static boolean checkDiskSpace(List<storage_domains>
storageDomains, Guid storagePoolId, List<String> messages) {
has no business in imagesHandler
s/checkDiskSpace/checkStorageDomainsFreeSpace/
....................................................
Commit Message
Line 7: ImagesHandler PerformImageChecks refactor (WIP)
Why?
--
To view, visit http://gerrit.ovirt.org/6197
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1a8a8312b8e1e31456ad7ee4f5d6edc96029e5ea
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
