Doron Fediuck has posted comments on this change. Change subject: Trusted Compute Pools - Open Attestation integration with oVirt engine proposal ......................................................................
Patch Set 2: I would prefer that you didn't submit this (6 inline comments) Hi Dave, although a step forward I still do not see the documented design for this patch. Am I missing something? I recommend you follow http://gerrit.ovirt.org/#/c/9023/ as a good example how such changes should be introduced. Additionally, Several comments from previous patch version are being disregarded. I'd assume the comments to be implemented if no good reason why not to implement them. Thanks! Doron .................................................... File backend/manager/modules/bll/pom.xml Line 178: <dependency> Line 179: <groupId>org.codehaus.jackson</groupId> Line 180: <artifactId>jackson-core-asl</artifactId> Line 181: <version>1.9.4</version> Line 182: </dependency> I second Juan, and also wonder- why did you ignore my comment in previous patch? The reason for the comment is to improve and adjust your addition. Please follow the general project conventions. Line 183: Line 184: </dependencies> Line 185: Line 186: <build> .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsSelector.java Line 518: vmNICs = getVmNetworkInterfaceDao().getAllForVm(getVm().getId()); Line 519: } Line 520: return vmNICs; Line 521: } Line 522: private static boolean validateHostIsTrusted(VDS curVds) { +1. Please see previous version of this patch. Line 523: String attestationWSURL, trustStorePath; Line 524: attestationWSURL = Config.<String> GetValue(ConfigValues.AttestationWebServicesUrl); Line 525: trustStorePath=Config.<String> GetValue(ConfigValues.TrustStore); Line 526: DefaultHttpClient httpclient = new DefaultHttpClient(); Line 533: try { Line 534: KeyStore trustStore = KeyStore.getInstance(KeyStore.getDefaultType()); Line 535: FileInputStream instream = new FileInputStream(new File(trustStorePath +"/TrustStore.jks")); Line 536: trustStore.load(instream, null); Line 537: SSLSocketFactory socketFactory = new SSLSocketFactory(trustStore); This code part is already commented to be duplicated. Please see how it should be changed to avoid duplication. Line 538: Scheme sch = new Scheme("https", 8443, socketFactory); Line 539: httpclient.getConnectionManager().getSchemeRegistry().register(sch); Line 540: HttpPost httpPost = new HttpPost(attestationWSURL +"/PollHosts"); Line 541: httpPost.setEntity(new StringEntity("{\"hosts\":[\"" +hostname +"\"]}")); Line 561: } Line 562: jParser.nextToken(); Line 563: if(jParser.getText().equalsIgnoreCase("trusted")) Line 564: flag=true; Line 565: System.out.println("host: " +hostname +" is " + jParser.getText()); +1. Please see previous version of this patch. Line 566: } Line 567: } catch (Exception e) { Line 568: e.printStackTrace(); Line 569: throw new RuntimeException(e.toString()); .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java Line 1337: AttestationWebServicesUrl(411), Line 1338: Line 1339: @TypeConverterAttribute(String.class) Line 1340: @DefaultValueAttribute("/etc/pki/ovirt-engine/certs") Line 1341: TrustStore(412), This will collide with TruststoreUrl, as others will fail to see the difference. If possible you can should TruststoreUrl. If not, please explain why. Either way the name should be meaningful. Line 1342: Line 1343: Invalid(65535); Line 1344: Line 1345: private int intValue; .................................................... File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmStaticDAODbFacadeImpl.java Line 83: .addValue("quota_id", vm.getQuotaId()) Line 84: .addValue("allow_console_reconnect", vm.isAllowConsoleReconnect()) Line 85: .addValue("cpu_pinning", vm.getCpuPinning()) Line 86: .addValue("host_cpu_flags", vm.isUseHostCpuFlags()) Line 87: .addValue("trust_lvl", vm.gettrust_lvl()); Any news on Omer's comment from previous patch version? Line 88: } Line 89: Line 90: @Override Line 91: public void remove(Guid id) { -- To view, visit http://gerrit.ovirt.org/11237 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4de780cd46069638433255f3f9c994575f752e55 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Dave Chen <[email protected]> Gerrit-Reviewer: Dave Chen <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Itamar Heim <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
