Liron Aravot has posted comments on this change.
Change subject: core: fix err msg-add disk to locked VM (#865551)
......................................................................
Patch Set 5: (3 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
Line 85: // if user sent drive check that its not in use
Line 86: returnValue = returnValue && (vm == null ||
isDiskCanBeAddedToVm(getParameters().getDiskInfo()));
Line 87: if (returnValue && DiskStorageType.IMAGE ==
getParameters().getDiskInfo().getDiskStorageType()) {
Line 88: returnValue = checkIfImageDiskCanBeAdded(vm);
Line 89: }
maybe put that in an else block to avoid the duplicate check of the disk
storage type.
Line 90: if (returnValue && DiskStorageType.LUN ==
getParameters().getDiskInfo().getDiskStorageType()) {
Line 91: returnValue = checkIfLunDiskCanBeAdded();
Line 92: }
Line 93:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
Line 457: if (checkIsValid && !isValid) {
Line 458: returnValue = false;
Line 459: ListUtils.nullSafeAdd(messages,
VdcBllMessages.ACTION_TYPE_FAILED_IMAGE_REPOSITORY_NOT_FOUND.toString());
Line 460: }
Line 461:
Shouldn't this happen inside the if? if returnValue is already false then we
don't need to load the images.
Line 462: List<DiskImage> images = getImages(vm, diskImageList);
Line 463: if (returnValue && checkImagesLocked) {
Line 464: List<String> lockedDisksAliases = new ArrayList<String>();
Line 465: for (DiskImage diskImage : images) {
Line 467: lockedDisksAliases.add(diskImage.getDiskAlias());
Line 468: returnValue = false;
Line 469: }
Line 470: }
Line 471: if (lockedDisksAliases.size() > 0) {
I think that we don't need to use nullSafeAdd and check that the list is
initiated every time..it could be done in the begining of the methiod, if the
list of messages is null then no message will return anyway..so i guess that
there's no reason for performing the checks :) fruthermore, we will probably
get NPE anyway in the place that executed this method when there will be a try
to check that list.
Line 472: ListUtils.nullSafeAdd(messages,
VdcBllMessages.ACTION_TYPE_FAILED_DISKS_ARE_LOCKED.toString());
Line 473: messages.add(String.format("$%1$s %2$s",
"diskAliases", StringUtils.join(lockedDisksAliases, ", ")));
Line 474: }
Line 475: if (returnValue && vm.getstatus() ==
VMStatus.ImageLocked) {
--
To view, visit http://gerrit.ovirt.org/8666
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7a82d91b0ee51ebd1a612dbf210e0ea4533e746
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
