Maor Lipchuk has posted comments on this change.
Change subject: backend, restapi: Add read-only disk functionality
......................................................................
Patch Set 14:
(6 comments)
partial reviewed, main critical comment is at line 581
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmFromConfigurationCommand.java
Line 70: private AuditLogType
attemptToAttachDisksToImportedVm(Collection<Disk> disks){
Line 71: List<String> failedDisks = new LinkedList<>();
Line 72: for (Disk disk : disks) {
Line 73: AttachDettachVmDiskParameters params = new
AttachDettachVmDiskParameters(getVm().getId(),
Line 74: disk.getId(), disk.getPlugged(),
disk.getReadOnly());
Is this well formatted?
Line 75: VdcReturnValueBase returnVal =
getBackend().runInternalAction(VdcActionType.AttachDiskToVm, params);
Line 76: if (!returnVal.getSucceeded()) {
Line 77: failedDisks.add(disk.getDiskAlias());
Line 78: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
Line 313: setSucceeded(true);
Line 314: return null;
Line 315: }
Line 316:
Line 317: private void updateDeviceProperties() {
who will use clearVmDeviceAddress now?
You should also delete clearVmDeviceAddress from vm_device_sp and
clearDeviceAddress from VMDeviceDaoDbFacade
also remove it from the tests UpdateVmDiskCommandTest VmDeviceDAOTest
Line 318: VmDevice device = getVmDeviceDao().get(new
VmDeviceId(getOldDisk().getId(), getVmId()));
Line 319: if
(!ObjectUtils.objectsEqual(getOldDisk().getReadOnly(),
getNewDisk().getReadOnly())) {
Line 320: device.setIsReadOnly(getNewDisk().getReadOnly());
Line 321: }
Line 315: }
Line 316:
Line 317: private void updateDeviceProperties() {
Line 318: VmDevice device = getVmDeviceDao().get(new
VmDeviceId(getOldDisk().getId(), getVmId()));
Line 319: if
(!ObjectUtils.objectsEqual(getOldDisk().getReadOnly(),
getNewDisk().getReadOnly())) {
Please use equals here instead of ObjectUtils, it uses a redundant check of null
Line 320: device.setIsReadOnly(getNewDisk().getReadOnly());
Line 321: }
Line 322: if (getOldDisk().getDiskInterface() !=
getNewDisk().getDiskInterface()) {
Line 323: device.setAddress(StringUtils.EMPTY);
Line 503: return !ObjectUtils.objectsEqual(oldQuotaId, getQuotaId());
Line 504: }
Line 505:
Line 506: private boolean shouldUpdateDeviceProperties() {
Line 507: return !ObjectUtils.objectsEqual(getOldDisk().getReadOnly(),
getNewDisk().getReadOnly()) ||
I don't really like the use of ObjectUtils.
if you got here then the disk is already part of VM so we know that readOnly
can not be null.
Why not simply use here
getOldDisk().getReadOnly() == getNewDisk().getReadOnly()
or
getOldDisk().getReadOnly().equals(getNewDisk().getReadOnly())
Line 508: getOldDisk().getDiskInterface() !=
getNewDisk().getDiskInterface();
Line 509: }
Line 510:
Line 511: private boolean isAtLeastOneVmIsNotDown(List<VM>
vmsDiskPluggedTo) {
Line 573: vmsDiskOrSnapshotPluggedTo.add(vm);
Line 574: }
Line 575:
Line 576: if (vm.getId().equals(getParameters().getVmId())) {
Line 577: vmDeviceForVm = pair.getSecond();
Suggestion(style issue): IMHO better to add an empty line before a new comment,
it makes the code more readable, but it is only a suggestion since it is not
obligated by the formatter
Line 578: // disk's 'readOnly' property is transient, more
than that disk can be read only in the scope of
Line 579: // one VM and read write in the scope of another,
therefore 'readOnly' should be updated with
Line 580: // the value from device.
Line 581:
getOldDisk().setReadOnly(vmDeviceForVm.getIsReadOnly());
Line 577: vmDeviceForVm = pair.getSecond();
Line 578: // disk's 'readOnly' property is transient, more
than that disk can be read only in the scope of
Line 579: // one VM and read write in the scope of another,
therefore 'readOnly' should be updated with
Line 580: // the value from device.
Line 581:
getOldDisk().setReadOnly(vmDeviceForVm.getIsReadOnly());
This is a bug
if the disk is shareable then the disk will be updated with the last VM in the
for loop.
Line 582: }
Line 583: }
Line 584: }
Line 585: }
--
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: 14
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: Liron Ar <[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
