Frank Kobzik has posted comments on this change. Change subject: engine: Phase 3: Configuring ConsoleOptions on backend ......................................................................
Patch Set 7: (6 comments) https://gerrit.ovirt.org/#/c/37974/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ConfigureConsoleOptionsQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ConfigureConsoleOptionsQuery.java: Line 53: Line 54: if (options.getVmId() == null) { Line 55: getQueryReturnValue().setExceptionString("VM id must be filled in console options."); Line 56: return false; Line 57: } > maybe check that getCachedVm() != null as well? (I didn't put it here as I was not sure if it's valid to touch the DB in validateInputs()). Let's move it, then. Line 58: Line 59: return true; Line 60: } Line 61: Line 60: } Line 61: Line 62: @Override Line 63: protected void executeQueryCommand() { Line 64: getQueryReturnValue().setSucceeded(true); > afair, this is not needed, queries are succeeded = true by defualt, you onl you're right. backend does that (I added simulating this behavior to test). Line 65: ConsoleOptions options = getParameters().getOptions(); Line 66: this.graphicsType = options.getGraphicsType(); Line 67: Line 68: if (!getCachedVm().isRunning()) { Line 103: GraphicsInfo graphicsInfo = getCachedVm().getGraphicsInfos().get(graphicsType); Line 104: Line 105: options.setSmartcardEnabled(getCachedVm().isSmartcardEnabled()); Line 106: options.setNumberOfMonitors(getCachedVm().getNumOfMonitors()); Line 107: options.setGuestHostName(getCachedVm().getVmHost().split("[ ]", -1)[0]); //$NON-NLS-1$ > please remove non-nls comment Done Line 108: if (graphicsInfo.getTlsPort() != null) { Line 109: options.setSecurePort(graphicsInfo.getTlsPort()); Line 110: } Line 111: Line 208: Line 209: return null; Line 210: } Line 211: Line 212: VM getCachedVm() { > private? I use this method for mocking in tests. Line 213: if (cachedVm == null) { Line 214: cachedVm = getBackend().runInternalQuery( Line 215: VdcQueryType.GetVmByVmId, Line 216: new IdQueryParameters(getParameters().getOptions().getVmId())).getReturnValue(); Line 212: VM getCachedVm() { Line 213: if (cachedVm == null) { Line 214: cachedVm = getBackend().runInternalQuery( Line 215: VdcQueryType.GetVmByVmId, Line 216: new IdQueryParameters(getParameters().getOptions().getVmId())).getReturnValue(); > probably need to pass the filter parameter from the parameters of this clas You're right, thanks! Line 217: } Line 218: Line 219: return cachedVm; Line 220: } https://gerrit.ovirt.org/#/c/37974/7/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConsoleOptionsParams.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConsoleOptionsParams.java: Line 1: package org.ovirt.engine.core.common.queries; Line 2: Line 3: import org.ovirt.engine.core.common.console.ConsoleOptions; Line 4: Line 5: public class ConsoleOptionsParams extends VdcQueryParametersBase { > where is this used? Oops, this should go to next patch. We have 2 params classes in this whole feature: 1, this one, 2, ConfigureConsoleOptions params, which extends 1 and adds setTicket attribute. 2, is used in ConfigureConsole options query and setTicket attribute is used for convenience to instruct backend to execute SetVmTicket and fill ticket attribute in options. As for 1, that is a simple wrapper around ConsoleOptions and is used in GetConsoleDescriptorQuery (next patch), which is really dumb query that turns ConsoleOptions instance into string (setTicket attribute is not needed here). This class should be probably moved to the next patch. Line 6: Line 7: ConsoleOptions options; Line 8: Line 9: public ConsoleOptionsParams() { } -- To view, visit https://gerrit.ovirt.org/37974 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida16e4de2701d6f6b3f6b3ed52eddca805fa43a7 Gerrit-PatchSet: 7 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: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
