Maor Lipchuk has posted comments on this change.
Change subject: core, restapi: Introducing support for attaching disk snapshot
......................................................................
Patch Set 36:
(3 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java
Line 117: }
Line 118:
Line 119: protected boolean isDiskCanBeAddedToVm(Disk diskInfo, VM vm) {
Line 120: if (!diskInfo.isDiskSnapshot()) {
Line 121: if (diskInfo.isBoot()) {
please use one if instead of nested ifs.
if (!diskInfo.isDiskSnapshot() && diskInfo.isBoot()) instead both ifs
Line 122: for (Disk disk : vm.getDiskMap().values()) {
Line 123: if (disk.isBoot() && !disk.isDiskSnapshot()) {
Line 124:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_BOOT_IN_USE);
Line 125: getReturnValue().getCanDoActionMessages().add(
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilderBase.java
Line 43: public int compare(Disk o1, Disk o2) {
Line 44: Boolean boot1 = o1.isBoot();
Line 45: Boolean boot2 = o2.isBoot();
Line 46: int bootResult = boot1.compareTo(boot2);
Line 47: if (bootResult == 0 && o1.isBoot() && o2.isBoot()) {
1. boot1 and boot2 should be primitive boolean.
2. bootResult should be equals to:
* boot1 == boot2 ? 0 : 1
3. Regarding the if condition I tend to agree with Arik that it is a bit
confusing,
I suggest to do something like this:
if (bootResult != 0) {
return bootRestult;
}
if (o1.isBoot() && o2.isBoot()) {
return Boolean.compare(o2.isDiskSnapshot(), o1.isDiskSnapshot());
}
This way it will still be efficient and more readable IMHO
Line 48: return Boolean.compare(o2.isDiskSnapshot(),
o1.isDiskSnapshot());
Line 49: }
Line 50: return bootResult;
Line 51: }
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmOldInfoBuilder.java
Line 70: .toLowerCase());
Line 71: drive.put("propagateErrors",
disk.getPropagateErrors().toString()
Line 72: .toLowerCase());
Line 73: switch (disk.getDiskInterface()) {
Line 74: case IDE:
No need for indentation here
Line 75: try {
Line 76: drive.put("if", "ide");
Line 77: drive.put("index",
String.valueOf(ideIndexSlots[ideCount]));
Line 78: ideCount++;
--
To view, visit http://gerrit.ovirt.org/17679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02579bf1a91cd294a5040acf432f1fdb87eb18c1
Gerrit-PatchSet: 36
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
