Omer Frenkel has posted comments on this change. Change subject: core: OsInfo-related changes ......................................................................
Patch Set 30: (3 comments) http://gerrit.ovirt.org/#/c/30838/30/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 322: protected boolean canDoAddVmCommand() { Line 323: boolean returnValue = false; Line 324: returnValue = areParametersLegal(getReturnValue().getCanDoActionMessages()); Line 325: // Check if number of monitors passed is legal Line 326: returnValue = returnValue && checkNumberOfMonitors(getParameters().graphicsTypesToBeSet()) why do you need to pass it as parameter? its accessible from the method as well (unless you are going to extract the method to some shared place later) Line 327: && checkSingleQxlDisplay(); Line 328: Line 329: returnValue = Line 330: returnValue http://gerrit.ovirt.org/#/c/30838/30/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java: Line 156: } Line 157: } Line 158: Line 159: // check graphics devices inherited from template Line 160: if (!VmHandler.isGraphicsAndDisplaySupported(getParameters().getVmStaticData().getOsId(), why here and not in addVmCommand? this command is the same but responsible for cloning Line 161: getEntityGraphicsTypes(getVmTemplateId()), Line 162: getParameters().getVm().getDefaultDisplayType(), Line 163: getReturnValue().getCanDoActionMessages(), Line 164: getVdsGroup().getcompatibility_version())) { http://gerrit.ovirt.org/#/c/30838/30/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 459: Line 460: Set<GraphicsType> getGraphicsTypesForVm(VM vm) { Line 461: Set<GraphicsType> graphicsTypes = new HashSet<>(); Line 462: Line 463: for (VmDevice vmDevice : DbFacade.getInstance().getVmDeviceDao().getVmDeviceByVmIdAndType(vm.getId(), VmDeviceGeneralType.GRAPHICS)) { how this works? on import vm the vm is not in the db, getting devices from db should return empty, no? i think you can use getVm().getManagedVmDeviceMap() (dont use vm from params) Line 464: graphicsTypes.add(GraphicsType.fromVmDeviceType(VmDeviceType.valueOf(vmDevice.getDevice()))); Line 465: } Line 466: Line 467: return graphicsTypes; -- 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: 30 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <fkob...@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
