Arik Hadas has posted comments on this change.
Change subject: core, restapi: Introducing support for attaching disk snapshot
......................................................................
Patch Set 34:
(7 comments)
the virt related stuff looks ok to me, but I suggest that omer will take a look
as well as he is more familiar with devices related flows than I am
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java
Line 119:
Line 120: protected boolean isDiskCanBeAddedToVm(Disk diskInfo, VM vm) {
Line 121: if (diskInfo.isBoot()) {
Line 122: for (Disk disk : vm.getDiskMap().values()) {
Line 123: if (disk.isBoot() &&
!(DiskStorageType.IMAGE.equals(disk.getDiskStorageType()) &&
((DiskImage)disk).isDiskSnapshot())) {
note that it is more common to use "==" instead of equals when it comes to enum
values in java because "==" check guarantee in compilation time that the two
operands are of the same type
Line 124:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_BOOT_IN_USE);
Line 125: getReturnValue().getCanDoActionMessages().add(
Line 126: String.format("$DiskName %1$s",
disk.getDiskAlias()));
Line 127: getReturnValue().getCanDoActionMessages().add(
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java
Line 251: vm.clearDisks();
Line 252: updateDisksForVm(vm, imageList);
Line 253: }
Line 254:
Line 255: private static void clearVmDisks(VM vm) {
if this method is not in use, it should be removed
Line 256: vm.getDiskList().clear();
Line 257: vm.getDiskMap().clear();
Line 258: }
Line 259:
Line 256: vm.getDiskList().clear();
Line 257: vm.getDiskMap().clear();
Line 258: }
Line 259:
Line 260: public static void updateDisksForVm(VM vm, Collection<? extends
Disk> diskList) {
'diskList' is now misleading, how about renaming it to 'disks' ?
Line 261: for (Disk disk : diskList) {
Line 262: if (disk.isAllowSnapshot()) {
Line 263: DiskImage image = (DiskImage) disk;
Line 264: vm.getDiskMap().put(image.getId(), image);
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAODbFacadeImpl.java
Line 70: createEntityRowMapper(), parameterSource);
Line 71: }
Line 72:
Line 73: @Override
Line 74: public List<VmDevice> getVmDevicesByDeviceId(Guid deviceId, Guid
vmId) {
this method should return one device or null right? if so it shouldn't return
list and its name is confusing
Line 75: MapSqlParameterSource parameterSource =
getCustomMapSqlParameterSource()
Line 76: .addValue("device_id", deviceId)
Line 77: .addValue("vm_id", vmId);
Line 78:
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilder.java
Line 301:
Line 302: addBootOrder(vmDevice, struct);
Line 303: struct.put(VdsProperties.Shareable,
Line 304: (vmDevice.getSnapshotId() != null &&
FeatureSupported.hotPlugDiskSnapshot(vm.getVdsGroupCompatibilityVersion())) ?
VdsProperties.Transient
Line 305: : disk.isShareable());
did you verify that it works without converting the isShareable to String?
Line 306: struct.put(VdsProperties.Optional,
Boolean.FALSE.toString());
Line 307: struct.put(VdsProperties.ReadOnly,
String.valueOf(vmDevice.getIsReadOnly()));
Line 308: struct.put(VdsProperties.SpecParams,
vmDevice.getSpecParams());
Line 309: struct.put(VdsProperties.DeviceId,
String.valueOf(vmDevice.getId().getDeviceId()));
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilderBase.java
Line 219: // order first by drive numbers and then order by boot for
the bootable
Line 220: // drive to be first (important for IDE to be index 0) !
Line 221: List<Disk> diskImages = new ArrayList<Disk>(vm.getDiskMap()
Line 222: .values());
Line 223: List<Pair<Disk, VmDevice>> toReturn = new ArrayList<>();
this field is not in use.. should be removed
Line 224: Collections.sort(diskImages, new
DiskImageByDiskAliasComparator());
Line 225: Collections.sort(diskImages,
Line 226: Collections.reverseOrder(new
DiskImageByBootComparator()));
Line 227: Collections.sort(diskImages, new Comparator<Disk>() {
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/AbstractDiskModel.java
Line 439: }
Line 440: }
Line 441:
Line 442: private boolean isDiskSnapshot(Disk disk) {
Line 443: return
(DiskStorageType.IMAGE.equals(disk.getDiskStorageType()) &&
((DiskImage)disk).isDiskSnapshot());
check using "==" is better here as well
btw, why not adding the 'isDiskSnapshot' to Disk with default implementation of
returning false instead of using such castings?
Line 444: }
Line 445:
Line 446: private void updateBootableDiskAvailable() {
Line 447: AsyncDataProvider.getVmDiskList(new AsyncQuery(this, new
INewAsyncCallback() {
--
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: 34
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
