Frank Kobzik has posted comments on this change. Change subject: core: add graphics device to management classes ......................................................................
Patch Set 8: (2 comments) http://gerrit.ovirt.org/#/c/28567/8/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 440: } Line 441: Line 442: private static int getNumOfMonitors(VM vm) { Line 443: int maxMonitorsSpice = vm.getSingleQxlPci() ? 1 : vm.getNumOfMonitors(); Line 444: int maxMonitorsVnc = Math.max(1, vm.getNumOfMonitors()); > please create a constant for VNC_MAX_MONITORS I'm not totally sure - maxMonitorsVnc is not constant - the only constant here is 1 - do you mean this one? Line 445: Line 446: return Math.min(maxMonitorsSpice, maxMonitorsVnc); Line 447: } Line 448: http://gerrit.ovirt.org/#/c/28567/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmManagementParametersBase.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmManagementParametersBase.java: Line 79: * This parameter is used to decide if to create graphics device or not. If it is null then: Line 80: * - for add vm don't add graphics device, Line 81: * - for update the current configuration will remain. Line 82: */ Line 83: private Map<GraphicsType, Boolean> shouldUpdateGraphicsDevice; > why having map<type,bool> and map<type,device> and not just That depends. I believe with list/set approach we'd lose the ability to handle the devices independently (you've got spice only VM, you want to add VNC graphics to it). In the situation above, you'd have to specify both the devices in xml/json whatever. Is that correct approach? I mean - the boolean flags you can see in this class are here for handling precisely this case. But if we discuss this approach with Juan and he agrees, I'm fine with it. Line 84: Line 85: public VmManagementParametersBase() { Line 86: init(); Line 87: } -- To view, visit http://gerrit.ovirt.org/28567 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I39c84ce94cd3cb52286c52d765a389e69c9b0e62 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@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
