Omer Frenkel has posted comments on this change. Change subject: core: Graphics Device CRUD ......................................................................
Patch Set 13: (5 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 make sure it exists, the first will just check user sent something, but not if this is a valid vm id 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: result.add(new GraphicsDevice(spiceDevs.get(0)); 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 have it as a ctor, so you can use: GraphicsDevice dev = new GraphicsDevice(someVmDevice) 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) 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
