Moti Asayag has posted comments on this change.
Change subject: core : Fixing ImportVmCommand scenario
......................................................................
Patch Set 1: (11 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportExportCommon.java
Line 19:
seems like a good candidate for a Validator method.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java
Line 80: imageList = new
ArrayList<DiskImage>(getVm().getDiskMap().values());
can't we get NPE here ? The parameters might contain null as VM value, the can
do action is being performed after the command is initialized and there is no
sort of validation that prevent the client from sending null for VM.
In this case - getVm().getDiskMap() will end with NPE.
Line 93: boolean retVal =
ImportExportCommon.checkStoragePool(getStoragePool(), getReturnValue()
getReturnValue().getCanDoActionMessages() is repeated several times here -
extract it to a local variable.
Line 101: StorageDomainValidator validator = new
StorageDomainValidator(storageDomain);
You may replace the validator call with CommandBase.validate(ValidationResult
validationResult) which keeps the code less verbose.
See additional comments in the StorageDomainValidator.java
Line 134: List<VM> vms = (List) qretVal.getReturnValue();
please add type for the casting list: (List<VM>)
Line 138: return
vm.getId().equals(getParameters().getVm().getId());
at this point you may replace it with getVmId()
Line 193: VmStatic duplicateVm =
getVmStaticDAO().get(getParameters().getVm().getId());
although out of patch scope, this also can be replaced with getVmId()
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java
Line 180: StorageDomainValidator validator = new
StorageDomainValidator(getStorageDomain(domainId));
see previous comment about CommandBase.validate()
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java
Line 305: if (imageToDestinationDomainMap.get(image.getId()) ==
null) {
if not using the return value of the Map.get() consider using Map.containsKey
instead.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveVmCommand.java
Line 242: for (DiskImage image : getVm().getDiskMap().values()) {
if not using the return value of the Map.get() consider using Map.containsKey
instead.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java
Line 30:
consider changing the return value to ValidationResult and to split this
validation method into granular checks, that way you can enhance the
CommandBase.validate(ValidationResult) from the canDoAction.
See SnapshotsValidator for example.
--
To view, visit http://gerrit.ovirt.org/2351
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc830aef5dfbd10b8061df08f0f353ec2e71a502
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
