ofri masad has posted comments on this change.
Change subject: Trusted Compute Pools - Open Attestation integration with oVirt
engine proposal
......................................................................
Patch Set 3: (8 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationCacheManager.java
Line 36: }
Line 37:
Line 38: public static AttestationValue getValueFromCache(String key){
Line 39: AttestationValue value = attestationValues.get(key);
Line 40: return value;
return statement can be one line: return attestationValues.get(key);
Line 41: }
Line 42:
Line 43: public static boolean isExpired(String key){
Line 44: AttestationValue value = getValueFromCache(key);
Line 43: public static boolean isExpired(String key){
Line 44: AttestationValue value = getValueFromCache(key);
Line 45: if (value != null && value.getVtime() != null){
Line 46: if (System.currentTimeMillis() -
value.getVtime().getTime() <= cacheTimeout)
Line 47: return false;
Please add braces
Line 48: }
Line 49: return true;
Line 50: }
Line 51:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationService.java
Line 39: //Deal with truststore in this method acceptable? If still collide
with current system, would you kindly provide some suggestion?
Line 40: public static HttpClient getClient(){
Line 41: HttpClient httpClient = new HttpClient();
Line 42: if (Config.<Boolean>
GetValue(ConfigValues.SecureConnectionWithOATServers)) {
Line 43: URL truststoreUrl;
truststoreUrl / trustStoreUrl
Line 44: try {
Line 45: int port = Config.<Integer>
GetValue(ConfigValues.AttestationPort);
Line 46: truststoreUrl = new URL("file://" +
Config.resolveAttestationTrustStorePath());
Line 47: String truststorePassword = Config.<String>
GetValue(ConfigValues.AttestationTruststorePass);
Line 77: }
Line 78: else{
Line 79: for (AttestationValue value :
attestHosts(AttestationCacheManager.getAllKeys())){
Line 80: AttestationCacheManager.updateCache(value);
Line 81: }
Each time one host is expired we refresh all of the hosts?
a. this could take a long time (can we at least call all the host in parallel?)
b. so why do we keep expiration for each AttestationValue?
Line 82: return
AttestationCacheManager.getValueFromCache(key).getTrustLevel();
Line 83: }
Line 84: }
Line 85: return result;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationValue.java
Line 55: return result;
Line 56: }
Line 57:
Line 58: @Override
Line 59: public boolean equals(Object obj) {
Please simplify this auto generated equals() - see how in this patch:
http://gerrit.ovirt.org/#/c/12342/
Line 60: if (this == obj)
Line 61: return true;
Line 62: if (obj == null || (obj.getClass() != this.getClass()))
Line 63: return false;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsSelector.java
Line 370:
Line 371: private ValidationResult validateHostIsReadyToRun(final VDS vds,
StringBuilder sb, boolean isMigrate) {
Line 372: // buffer the mismatches as we go
Line 373: sb.append(" VDS ").append(vds.getvds_name()).append("
").append(vds.getId()).append(" ");
Line 374: if (!AttestationCacheManager.exists(vds.gethost_name())){
please fix formatting
Line 375: AttestationValue newValue = new AttestationValue();
Line 376: newValue.setHostName(vds.gethost_name());
Line 377: newValue.setTrustLevel(AttestationResult.UNKNOWN);
Line 378: AttestationCacheManager.addToCache(newValue);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AttestationResult.java
Line 4: public enum AttestationResult {
Line 5: UNTRUSTED(0),
Line 6: TRUSTED(1),
Line 7: UNKNOWN(2),
Line 8: TIMEOUT(3);
If I'm not mistaken, this enum is never saved in DB. So a basic enum is enough
(no need for all of the part from line 10 onward).
If you do plan to use it in the VM table in the DB and you want to save the int
and not the string (currently a string is saved) then this mapping will be
needed.
Line 9:
Line 10: private int intValue;
Line 11: private static java.util.HashMap<Integer, AttestationResult>
mappings = new HashMap<Integer, AttestationResult>();
Line 12:
....................................................
Commit Message
Line 3: AuthorDate: 2013-01-19 05:23:18 -0500
Line 4: Commit: Dave Chen <wei.d.c...@intel.com>
Line 5: CommitDate: 2013-02-27 05:17:53 -0500
Line 6:
Line 7: Trusted Compute Pools - Open Attestation integration with oVirt engine
proposal
Could you please provide an explanation (or better, a wiki page) about the
feature.
Line 8:
Line 9: Change-Id: I4de780cd46069638433255f3f9c994575f752e55
--
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: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dave Chen <wei.d.c...@intel.com>
Gerrit-Reviewer: Dave Chen <wei.d.c...@intel.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: ofri masad <oma...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
