Francesco Romani has posted comments on this change. Change subject: core, engine: Services for providing info to the Console Proxy ......................................................................
Patch Set 4: (8 comments) partial response to comments. Next upload will still be WIP with known issues. https://gerrit.ovirt.org/#/c/35887/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/aaa/GetAllUserProfilesQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/aaa/GetAllUserProfilesQuery.java: > this is not aaa moved into bll Line 1: package org.ovirt.engine.core.bll.aaa; Line 2: Line 3: import org.ovirt.engine.core.bll.QueriesCommandBase; Line 4: import org.ovirt.engine.core.common.queries.VdcQueryParametersBase; https://gerrit.ovirt.org/#/c/35887/4/backend/manager/modules/services/src/main/java/org/ovirt/engine/core/services/ConsoleProxyServlet.java File backend/manager/modules/services/src/main/java/org/ovirt/engine/core/services/ConsoleProxyServlet.java: > VMConsoleProxyServlet will match the ovirt-vmconsole package :) renamed as suggested Line 1: package org.ovirt.engine.core.services; Line 2: Line 3: import org.apache.commons.lang.StringUtils; Line 4: import org.codehaus.jackson.map.ObjectMapper; Line 83: Line 84: for (VM vm : vmsList) { Line 85: IdQueryParameters vmParam = new IdQueryParameters(vm.getId()); Line 86: Line 87: VdcQueryReturnValue retAddr = backend.runInternalQuery(VdcQueryType.GetManagementInterfaceAddressByVmId, vmParam); > probably better to have one query to perform the entire sequence instead of Noted, not done yet, added TODO Line 88: Line 89: if (retAddr != null && retAddr.getReturnValue() != null) { Line 90: String vdsAddress = (String) retAddr.getReturnValue(); Line 91: Line 119: @Override Line 120: protected void doPost(HttpServletRequest request, HttpServletResponse response) Line 121: throws ServletException, IOException { Line 122: Line 123: response.setContentType("application/json"); > set content type only when success and you actually sending content. Done Line 124: Line 125: OutputStream outputStream = response.getOutputStream(); Line 126: Line 127: String stringParameters = null; Line 126: Line 127: String stringParameters = null; Line 128: Line 129: try { Line 130: stringParameters = validateTicket(readBody(request.getReader())); > much better to use inputstream and copy byte by byte instead of using reade Noted, not fixed yet, added TODO Line 131: } catch (GeneralSecurityException e) { Line 132: log.error("Error validating ticket: ", e); Line 133: response.setStatus(HttpURLConnection.HTTP_FORBIDDEN); Line 134: } catch (IOException e) { Line 133: response.setStatus(HttpURLConnection.HTTP_FORBIDDEN); Line 134: } catch (IOException e) { Line 135: log.error("Error decoding ticket: ", e); Line 136: response.setStatus(HttpURLConnection.HTTP_INTERNAL_ERROR); Line 137: } > this try/catch should be for entire function, make sure you do not continue Done Line 138: Line 139: ObjectMapper mapper = new ObjectMapper(); Line 140: Line 141: Map<String, String> parameters = mapper.readValue( Line 157: result = availableConsoles(backend, userId); Line 158: } else if ("public_keys".equals(command)) { Line 159: result = publicKeys(backend); Line 160: } Line 161: } > else? raised error on both cases (mismatched version, unknown command) Line 162: Line 163: mapper.writeValue(outputStream, result); Line 164: Line 165: } catch (Exception e) { Line 165: } catch (Exception e) { Line 166: response.setStatus(HttpURLConnection.HTTP_INTERNAL_ERROR); Line 167: log.error("Error processing request: ", e); Line 168: } finally { Line 169: outputStream.close(); > try with resources? Done Line 170: } Line 171: } -- To view, visit https://gerrit.ovirt.org/35887 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53c721da21cefcf4069d14c7016b6f7d97f9eac9 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vitor de Lima <vdel...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <wallaroo1...@gmail.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
