Frank Kobzik has posted comments on this change. Change subject: core: Graphics Device CRUD ......................................................................
Patch Set 8: (6 comments) http://gerrit.ovirt.org/#/c/25409/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractGraphicsDeviceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractGraphicsDeviceCommand.java: Line 11: public abstract class AbstractGraphicsDeviceCommand<T extends GraphicsParameters> extends CommandBase<T> { Line 12: Line 13: protected AbstractGraphicsDeviceCommand(T parameters) { Line 14: super(parameters); Line 15: setVmId(parameters.getDev().getVmId()); > maybe you can do something like Done Line 16: } Line 17: Line 18: @Override Line 19: protected boolean canDoAction() { http://gerrit.ovirt.org/#/c/25409/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetGraphicsDevicesQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetGraphicsDevicesQuery.java: Line 17: @Override Line 18: protected void executeQueryCommand() { Line 19: List<GraphicsDevice> result = new LinkedList<>(); Line 20: Line 21: // we must use getVmDeviceByVmIdTypeAndDevice since it supports user filtering > thats ok, but we can also create a query for getVmDeviceByVmIdAndType to su I'd prefer not to do it in scope of spice+vnc patch. I think the change is not that trivial. Line 22: List<VmDevice> spiceDevs = getDbFacade().getVmDeviceDao().getVmDeviceByVmIdTypeAndDevice( Line 23: getParameters().getId(), Line 24: VmDeviceGeneralType.GRAPHICS, Line 25: VmDeviceType.SPICE.getName(), Line 25: VmDeviceType.SPICE.getName(), Line 26: getUserID(), Line 27: getParameters().isFiltered()); Line 28: if (spiceDevs != null && !spiceDevs.isEmpty()) { Line 29: result.add(GraphicsDevice.fromVmDevice(spiceDevs.get(0))); > wil we have GRAPHICS device per type? Yes, if users want SPICE + VNC @ the same time, we'll have 2 devs (which corresponds to libvirt). Multimonitors are more question of VIDEO devices. We can restrict SPICE + VNC option to allow single VIDEO device, but afaik VNC vm can start with multiple VIDEO devices. Line 30: } Line 31: Line 32: List<VmDevice> vncDevs = getDbFacade().getVmDeviceDao().getVmDeviceByVmIdTypeAndDevice( Line 33: getParameters().getId(), http://gerrit.ovirt.org/#/c/25409/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateGraphicsDeviceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateGraphicsDeviceCommand.java: Line 27: } Line 28: return result; Line 29: } Line 30: Line 31: boolean entityExists() { > i think this can be saved with my suggestion in the abstract command, no? Done. Line 32: if (getParameters().isVm()) { Line 33: Guid vmId = getParameters().getDev().getVmId(); Line 34: return vmId != null && getVmDAO().get(vmId) != null; Line 35: } else { http://gerrit.ovirt.org/#/c/25409/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GraphicsDevice.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GraphicsDevice.java: Line 12: Line 13: static { Line 14: typeToId = new HashMap<VmDeviceType, Guid>(); Line 15: typeToId.put(VmDeviceType.SPICE, Guid.createGuidFromString("00000000-0000-0000-0000-000000000000")); // $NON-NLS-1$ Line 16: typeToId.put(VmDeviceType.VNC, Guid.createGuidFromString("00000000-0000-0000-0000-000000000001")); // $NON-NLS-1$ > once you hard code a string, you know something is not right.. We actually don't allow multiple graphics devs of same type (like 2 spice framebuffers). So the device id is redundant here. But of course, I can rewrite it to use new id, if you want :) Line 17: } Line 18: Line 19: Line 20: private GraphicsDevice() { } http://gerrit.ovirt.org/#/c/25409/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GraphicsType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GraphicsType.java: Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: import org.ovirt.engine.core.common.utils.VmDeviceType; Line 4: Line 5: public enum GraphicsType { > this is not overlapping with DisplayType enum? This is more difficult question. In past DisplayType determined both graphics and video. The intention of this patch series is to separate graphics from video. - So the DisplayType will only determine video device (qxl, vga, cirrus) (I also tried to rename the enum in separate patch, but the change was quite big, so I ditched the patch (at least for the moment)). - New GraphicsType will determine graphics framebuffer (spice, vnc). In theory, displayType and defaultDisplayType could (and maybe should) be removed from VM/VmStatic/VmDynamic entities, but there are some facts that make it hard/impossible: - displayType att is used in 'Run Once'. If I remove it, I'll break Run Once (temporary vm configuration for run once is saved in VmDynamic - I've seen this pattern in other cases (e.g. vncKeyboardLayout does it the same way)) - defaultDisplayType could be removed in theory (its value is represented by type of VIDEO device), but it's a big change, so I kept it as is for now. Line 6: Line 7: SPICE(VmDeviceType.SPICE), Line 8: VNC(VmDeviceType.VNC); Line 9: -- To view, visit http://gerrit.ovirt.org/25409 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If9eed1ddb4aa8e8376ba5eff662f1bdf49fda800 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: [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
