Frank Kobzik has posted comments on this change. Change subject: core: Graphics Device CRUD ......................................................................
Patch Set 13: (5 comments) Thanks for comments. http://gerrit.ovirt.org/#/c/25409/13/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 26: if (dev == null) { Line 27: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DEVICE_MUST_BE_SPECIFIED); Line 28: } Line 29: Line 30: if (getParameters().isVm() && getVmId() == null) { > why getVmId and not getVm() ? the later will get the vm from the db and mak When adding a new vm, the VM doesn't exist in the db at this moment. The solution could be to override this in AddGraphicsDeviceCommand and don't call super. But then - this method would be used in UpdateGraphicsDeviceCommand only. It'd make more sense to me to enrich canDoAction of update command to check for VM/Template existence there. Line 31: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND); Line 32: } Line 33: Line 34: if (!getParameters().isVm() && getVmTemplateId() == null) { Line 30: if (getParameters().isVm() && getVmId() == null) { Line 31: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND); Line 32: } Line 33: Line 34: if (!getParameters().isVm() && getVmTemplateId() == null) { > same ^ Line 35: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_TEMPLATE_DOES_NOT_EXIST); Line 36: } Line 37: Line 38: if (dev.getGraphicsType() == null) { http://gerrit.ovirt.org/#/c/25409/13/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 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))); > see comment in GraphicsDevice class, here you can use: Done Line 30: } Line 31: Line 32: List<VmDevice> vncDevs = getDbFacade().getVmDeviceDao().getVmDeviceByVmIdTypeAndDevice( Line 33: getParameters().getId(), http://gerrit.ovirt.org/#/c/25409/13/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 21: public GraphicsType getGraphicsType() { Line 22: return GraphicsType.fromString(getDevice()); Line 23: } Line 24: Line 25: public static GraphicsDevice fromVmDevice(VmDevice vmDev) { > do you have to have it as a static method? if not, it would be nicer to hav Done. Line 26: GraphicsDevice dev = new GraphicsDevice(GraphicsType.fromString(vmDev.getDevice()).getCorrespondingDeviceType()); Line 27: dev.setId(vmDev.getId()); Line 28: return dev; Line 29: } Line 23: } Line 24: Line 25: public static GraphicsDevice fromVmDevice(VmDevice vmDev) { Line 26: GraphicsDevice dev = new GraphicsDevice(GraphicsType.fromString(vmDev.getDevice()).getCorrespondingDeviceType()); Line 27: dev.setId(vmDev.getId()); > please copy all vmDevice fields, some are very important (like address) Actually, graphics device does not have address element. The only thing that is important is the 'device' att (SPICE vs. VNC) and device id (and maybe specPars, if we used it for something). Line 28: return dev; Line 29: } Line 30: Line 31: } -- 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: 13 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
