Maor Lipchuk has posted comments on this change.
Change subject: core: Add read-only disk functionality
......................................................................
Patch Set 6:
(7 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
Line 483: }
Line 484: }
Line 485:
Line 486: private boolean isDiskReadOnly() {
Line 487: return
BooleanUtils.toBooleanDefaultIfNull(getParameters().getDiskInfo().getReadOnly(),
false);
Please don't use BooleanUtils?
You should use Boolean.valueOf(getParameters().getDiskInfo().getReadOnly())
Please see
http://docs.oracle.com/javase/7/docs/api/java/lang/Boolean.html#getBoolean%28java.lang.String%29
Line 488: }
Line 489:
Line 490: @Override
Line 491: protected VdcActionType getChildActionType() {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Disk.java
Line 25: private ArrayList<String> vmNames;
Line 26:
Line 27: /**
Line 28: * plugged and readOnly are of type Boolean (as opposed to
boolean) since they are optional
Line 29: * in case the disk is not in a vm context. In that case, null
will ensure they's invisible.
1. "they's invisible"?
2. There is an indentation extra space before the "in case"
Line 30: */
Line 31: private Boolean plugged;
Line 32: private Boolean readOnly;
Line 33:
Line 72: final int prime = 31;
Line 73: int result = super.hashCode();
Line 74: result = prime * result + ((plugged == null) ? 0 :
plugged.hashCode());
Line 75: result = prime * result + ((readOnly == null) ? 0 :
readOnly.hashCode());
Line 76: result = prime * result + ((vmEntityType == null) ? 0 :
vmEntityType.hashCode());
vmEntityType is not related to the change, it should be in another patch
Line 77: result = prime * result + ((vmNames == null) ? 0 :
vmNames.hashCode());
Line 78: result = prime * result + ((vmEntityType == null) ? 0 :
vmEntityType.hashCode());
Line 79: result = prime * result + numberOfVms;
Line 80: return result;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDevice.java
Line 62:
Line 63: /**
Line 64: * The device read-only flag
Line 65: */
Line 66: private Boolean isReadOnly;
After discussed it f2f with Sergey Allon and Tal, it was decided that for now
we will leave it as isPlugged, but will re-think about it in the future
Line 67:
Line 68: /**
Line 69: * The device alias.
Line 70: */
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmDiskResource.java
Line 81: @Override
Line 82: public Response deactivate(Action action) {
Line 83: HotPlugDiskToVmParameters params =
Line 84: new
HotPlugDiskToVmParameters(((BackendVmDisksResource) collection).parentId,
Line 85: guid); // deactivate doesn't care about
readOnly value.
I'm not sure what this comment means here
Line 86: return doAction(VdcActionType.HotUnPlugDiskFromVm, params,
action);
Line 87: }
Line 88:
Line 89: @Override
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java
Line 1189: 0,
Line 1190: o == null ? new HashMap<String, Object>() :
(Map<String, Object>) o,
Line 1191: false,
Line 1192: true,
Line 1193: isReadOnly,
You use this attribute only once, I would simply write here
getDiskReadOnly(device)
Line 1194: alias,
Line 1195: null);
Line 1196: newVmDevices.add(newDevice);
Line 1197: log.debugFormat("New device was marked for adding to VM
{0} Devices : {1}", vmId, newDevice);
Line 1202:
Line 1203: private boolean getDiskReadOnly(Map device) {
Line 1204: String readOnly = StringUtils.defaultIfEmpty(
Line 1205: (String) device.get(VdsProperties.Type),
Boolean.FALSE.toString());
Line 1206: return Boolean.getBoolean(readOnly);
Why do you need to use defaultIfEmpty?
Boolean.getBoolean(readOnly) will return false in case of null/empty or
anything that is different from true.
I would change it to return Boolean.getBoolean((String)
device.get(VdsProperties.Type));
or even better remove it entirely and simply pass this as an argument when
calling new VmDevice
Line 1207: }
Line 1208:
Line 1209: /**
Line 1210: * gets the device id from the structure returned by VDSM device
ids are stored in specParams map
--
To view, visit http://gerrit.ovirt.org/15409
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I124176e8feb91b601a71e76dd63051648ec4353a
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[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
