Roy Golan has posted comments on this change. Change subject: core: refactored graphics and display ......................................................................
Patch Set 2: (6 comments) http://gerrit.ovirt.org/#/c/30838/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 1334: // Choose a proper default display type according to the cluster architecture Line 1335: if (getParameters().getVmStaticData().getOsId() != OsRepository.AUTO_SELECT_OS && Line 1336: getParameters().getVmStaticData().getDefaultDisplayType() == null) { Line 1337: DisplayType defaultDisplayType = Line 1338: osRepository.getGraphicsAndDisplays(getParameters().getVmStaticData().getOsId(), maybe its time for a default display type method. Line 1339: getVdsGroup().getcompatibility_version()).get(0).getSecond(); Line 1340: Line 1341: getParameters().getVmStaticData().setDefaultDisplayType(defaultDisplayType); Line 1342: } http://gerrit.ovirt.org/#/c/30838/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java: Line 436 Line 437 Line 438 Line 439 Line 440 kudus for following your TODOs :) Line 453: Line 454: return true; Line 455: } Line 456: Line 457: Set<GraphicsType> getGraphicsTypesForVm(VM vmFromParams) { why did you do that delegation? Line 458: return VmDeviceUtils.getGraphicsTypesForVm(vmFromParams); Line 459: } Line 460: Line 461: /** http://gerrit.ovirt.org/#/c/30838/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java: Line 652: // Clear the first user: Line 653: getVm().setConsoleUserId(null); Line 654: Line 655: getVm().setDisplayType(getVm().getDefaultDisplayType()); // todo handle run once. Line 656: I assume further patch is handling this? Line 657: if (getParameters().getInitializationType() == null) { Line 658: // if vm not initialized, use sysprep/cloud-init Line 659: if (!getVm().isInitialized()) { Line 660: VmHandler.updateVmInitFromDB(getVm().getStaticData(), false); http://gerrit.ovirt.org/#/c/30838/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java: Line 1163: Line 1164: return null; Line 1165: } Line 1166: Line 1167: public static Set<GraphicsType> getGraphicsTypesForVm(VM vm) { not very test friendly. its a static helper that has a dao access. please change that Line 1168: Set<GraphicsType> graphicsTypes = new HashSet<>(); Line 1169: Line 1170: for (VmDevice vmDevice : dao.getVmDeviceByVmIdAndType(vm.getId(), VmDeviceGeneralType.GRAPHICS)) { Line 1171: graphicsTypes.add(GraphicsType.fromVmDeviceType(VmDeviceType.valueOf(vmDevice.getDevice()))); http://gerrit.ovirt.org/#/c/30838/2/packaging/conf/osinfo-defaults.properties File packaging/conf/osinfo-defaults.properties: Line 50: os.other.resources.minimum.ram.value = 256 Line 51: os.other.resources.maximum.ram.value = 64000 Line 52: os.other.resources.minimum.disksize.value = 1 Line 53: os.other.resources.minimum.numberOsCpus.value = 1 Line 54: os.other.devices.display.protocols.value = spice/qxl,vnc/qxl,vnc/cirrus finally this makes sense. PROTOCOL/DEVICE :) Line 55: os.other.devices.watchdog.models.value = i6300esb Line 56: Line 57: os.other.devices.disk.hotpluggableInterfaces.value = VirtIO_SCSI, VirtIO Line 58: os.other.devices.disk.hotpluggableInterfaces.value.3.0 = -- To view, visit http://gerrit.ovirt.org/30838 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0c7126308ab4e5e5418d804a8550a50e9994894 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
