Liron Ar has posted comments on this change.
Change subject: core: Add a validation when deactivate ISO domain.
......................................................................
Patch Set 4:
(6 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DeactivateStorageDomainCommand.java
Line 126: .isEmpty()) {
Line 127:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DETECTED_ACTIVE_VMS);
Line 128: return false;
Line 129: }
Line 130: if (getStorageDomain().getStorageDomainType() ==
StorageDomainType.ISO) {
this check should be before the one before, otherwise user might go and power
off his vms just to fail on that validation and i'd move them both to be after
the tasks check but that's regardless.
Line 131: addWarningForVmsWithAttachedDisk();
Line 132: }
Line 133: if (getStoragePool().getspm_vds_id() != null) {
Line 134: // In case there are running tasks in the pool, it is
impossible to deactivate the master storage domain
Line 154: vmNames.add(vmStatic.getName());
Line 155: }
Line 156: }
Line 157: if (!vmNames.isEmpty()) {
Line 158:
getReturnValue().getCanDoActionMessages().add(String.format("$VmNames %s",
StringUtils.join(vmNames, ",")));
1. why not addCanDoActionMessage?
2. IIRC you should change the order between those two lines so the message will
be added before the replacement.
Line 159:
addCanDoActionMessage(VdcBllMessages.ERROR_CANNOT_DEACTIVATE_DOMAIN_WITH_TASKS);
Line 160: }
Line 161: }
Line 162:
Line 155: }
Line 156: }
Line 157: if (!vmNames.isEmpty()) {
Line 158:
getReturnValue().getCanDoActionMessages().add(String.format("$VmNames %s",
StringUtils.join(vmNames, ",")));
Line 159:
addCanDoActionMessage(VdcBllMessages.ERROR_CANNOT_DEACTIVATE_DOMAIN_WITH_TASKS);
the message is wrong.
Line 160: }
Line 161: }
Line 162:
Line 163: /**
Line 383: .getVmDeviceByVmIdTypeAndDevice(vmId,
Line 384: VmDeviceGeneralType.DISK,
Line 385: VmDeviceType.CDROM.getName());
Line 386: for (VmDevice cdDevice : cdList) {
Line 387: String path = (String)
cdDevice.getSpecParams().get("path");
why not just containskey? when we'll have the key and null/""?
Line 388: if (path != null && path != "") {
Line 389: return false;
Line 390: }
Line 391: }
Line 394: .getVmDeviceDao()
Line 395: .getVmDeviceByVmIdTypeAndDevice(vmId,
Line 396: VmDeviceGeneralType.DISK,
Line 397: VmDeviceType.FLOPPY.getName());
Line 398: for (VmDevice cdDevice : floppyList) {
rename cdDevice, this is the floppy loop
Line 399: String path = (String)
cdDevice.getSpecParams().get("path");
Line 400: if (path != null && path != "") {
Line 401: return false;
Line 402: }
Line 396: VmDeviceGeneralType.DISK,
Line 397: VmDeviceType.FLOPPY.getName());
Line 398: for (VmDevice cdDevice : floppyList) {
Line 399: String path = (String)
cdDevice.getSpecParams().get("path");
Line 400: if (path != null && path != "") {
why not just containskey? when we'll have the key and null/""?
Line 401: return false;
Line 402: }
Line 403: }
Line 404: return true;
--
To view, visit http://gerrit.ovirt.org/20331
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I47c1a8155762ecd0b04bb17676151946982bb919
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@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
