Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2367523002:
Merge LogDnsClient's QueryLeafIndex and QueryAuditProof methods (Closed)
|
Created:
4 years, 3 months ago by Rob Percival Modified:
4 years, 2 months ago CC:
chromium-reviews, rsleevi+watch_chromium.org, certificate-transparency-chrome_googlegroups.com, Eran Messeri Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMerge LogDnsClient's QueryLeafIndex and QueryAuditProof methods
QueryAuditProof now takes a leaf hash, rather than a leaf index.
It will look up the leaf index, then the audit proof.
BUG=624894
Committed: https://crrev.com/8da709c5cf63b5535f8c30ae6cc9e786bc8eaae8
Cr-Commit-Position: refs/heads/master@{#424758}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Hide implementation details #Patch Set 3 : Rebase #Patch Set 4 : Bug fix #
Total comments: 6
Patch Set 5 : Addresses Ryan's comments #Patch Set 6 : Deletes some duplicated documentation #Patch Set 7 : Rebase #Patch Set 8 : Rebase #Patch Set 9 : Rebase #
Dependent Patchsets: Messages
Total messages: 55 (46 generated)
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
robpercival@chromium.org changed reviewers: + eranm@chromium.org
First draft of unifying the two methods into one. All of the tests pass, but I haven't had the time to look over it thoroughly. The next step after this will be returning some error codes synchronously, then implementation of the proper async-sync state machine.
Description was changed from ========== Enter a description of the change. Merge LogDnsClient's QueryLeafIndex and QueryAuditProof methods QueryAuditProof now takes a leaf hash, rather than a leaf index. It will look up the leaf index, then the audit proof. BUG=624894 ========== to ========== Merge LogDnsClient's QueryLeafIndex and QueryAuditProof methods QueryAuditProof now takes a leaf hash, rather than a leaf index. It will look up the leaf index, then the audit proof. BUG=624894 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Looks good at a first glance, only real issue to address is whether the tree_size and domain_for_log should be fields of Query or not. It seems to me they are bound to the first callback just to be available to the second one (and then repeatedly at the second one in case follow-up queries are needed). Since these are a part of the request metadata it would seem simpler to me to store them in the Query, but there may be counter-arguments I'm not aware of. https://codereview.chromium.org/2367523002/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2367523002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:142: QueryLeafIndex(domain_for_log, leaf_hash, tree_size, callback); Nit: domain_for_log and tree_size seem to always be passed around together. You could store both in the Query struct or have a separate struct for passing in the two. Not sure how much it's worth the effort though. https://codereview.chromium.org/2367523002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:166: weak_ptr_factory_.GetWeakPtr(), domain_for_log, tree_size); Here it seems like the tree_size and domain_for_log could be stored in the Query - from what I can tell they are bound to the callback just so that they're available for the follow-up query. https://codereview.chromium.org/2367523002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:232: QueryAuditProofNodes(std::move(proof), domain_for_log, tree_size, 0, Document the 0: tree_size, 0 /* start node index */, ... https://codereview.chromium.org/2367523002/diff/1/components/certificate_tran... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2367523002/diff/1/components/certificate_tran... components/certificate_transparency/mock_log_dns_traffic.h:100: void ExpectLeafIndexRequestAndResponse(base::StringPiece qname, Given the method you've added I suggest removing this one, as I can't conceivably see why anyone would prefer to specify a numerical value as a string.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/2367523002/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client.h (right): https://codereview.chromium.org/2367523002/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.h:108: const net::DnsResponse* response); Can these four methods be encapsulated into an internal implementation? It seems like they could - that is, the only sort of private structures necessary are the dns_client and maybe the bound_net_log, but those could be passed internally in the constructor of the internal "Query" object - because they're guaranteed to outlive the query (c.f. 126ish)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. The ultimate version of this is in https://codereview.chromium.org/2369373002/, which adds synchronous return values, implements the DoLoop pattern and makes other small improvements. https://chromiumcodereview-hr.appspot.com/2367523002/diff/1/components/certif... File components/certificate_transparency/log_dns_client.cc (right): https://chromiumcodereview-hr.appspot.com/2367523002/diff/1/components/certif... components/certificate_transparency/log_dns_client.cc:142: QueryLeafIndex(domain_for_log, leaf_hash, tree_size, callback); On 2016/09/23 12:59:31, Eran Messeri wrote: > Nit: domain_for_log and tree_size seem to always be passed around together. > You could store both in the Query struct or have a separate struct for passing > in the two. > Not sure how much it's worth the effort though. Done. https://chromiumcodereview-hr.appspot.com/2367523002/diff/1/components/certif... components/certificate_transparency/log_dns_client.cc:166: weak_ptr_factory_.GetWeakPtr(), domain_for_log, tree_size); On 2016/09/23 12:59:31, Eran Messeri wrote: > Here it seems like the tree_size and domain_for_log could be stored in the Query > - from what I can tell they are bound to the callback just so that they're > available for the follow-up query. Done. https://chromiumcodereview-hr.appspot.com/2367523002/diff/1/components/certif... components/certificate_transparency/log_dns_client.cc:232: QueryAuditProofNodes(std::move(proof), domain_for_log, tree_size, 0, On 2016/09/23 12:59:32, Eran Messeri wrote: > Document the 0: > tree_size, 0 /* start node index */, ... Done. https://chromiumcodereview-hr.appspot.com/2367523002/diff/1/components/certif... File components/certificate_transparency/log_dns_client.h (right): https://chromiumcodereview-hr.appspot.com/2367523002/diff/1/components/certif... components/certificate_transparency/log_dns_client.h:108: const net::DnsResponse* response); On 2016/09/23 21:59:29, Ryan Sleevi (slow) wrote: > Can these four methods be encapsulated into an internal implementation? It seems > like they could - that is, the only sort of private structures necessary are the > dns_client and maybe the bound_net_log, but those could be passed internally in > the constructor of the internal "Query" object - because they're guaranteed to > outlive the query (c.f. 126ish) Done. https://chromiumcodereview-hr.appspot.com/2367523002/diff/1/components/certif... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://chromiumcodereview-hr.appspot.com/2367523002/diff/1/components/certif... components/certificate_transparency/mock_log_dns_traffic.h:100: void ExpectLeafIndexRequestAndResponse(base::StringPiece qname, On 2016/09/23 12:59:32, Eran Messeri wrote: > Given the method you've added I suggest removing this one, as I can't > conceivably see why anyone would prefer to specify a numerical value as a > string. I hadn't actually meant to add this one - I was just toying with it during testing. The version that takes the leaf index as a string is actually more useful, as it lets you provide malformed responses.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Patchset #3 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2367523002/diff/140001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2367523002/diff/140001/components/certificate... components/certificate_transparency/log_dns_client.cc:87: class LogDnsClient::AuditProofQuery { See comments on the other CL about splitting definition/declaration. https://codereview.chromium.org/2367523002/diff/140001/components/certificate... components/certificate_transparency/log_dns_client.cc:110: proof_.reset(new net::ct::MerkleAuditProof); base::MakeUnique<> ? https://codereview.chromium.org/2367523002/diff/140001/components/certificate... components/certificate_transparency/log_dns_client.cc:344: // already at its limit (as specified by |max_concurrent_queries_|. Because you're asynchronously invoking this callback, this comment does not make sense.
Patchset #5 (id:160001) has been deleted
https://codereview.chromium.org/2367523002/diff/140001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2367523002/diff/140001/components/certificate... components/certificate_transparency/log_dns_client.cc:87: class LogDnsClient::AuditProofQuery { On 2016/10/03 23:51:07, Ryan Sleevi (slow) wrote: > See comments on the other CL about splitting definition/declaration. Done. https://codereview.chromium.org/2367523002/diff/140001/components/certificate... components/certificate_transparency/log_dns_client.cc:110: proof_.reset(new net::ct::MerkleAuditProof); On 2016/10/03 23:51:08, Ryan Sleevi (slow) wrote: > base::MakeUnique<> ? Done. https://codereview.chromium.org/2367523002/diff/140001/components/certificate... components/certificate_transparency/log_dns_client.cc:344: // already at its limit (as specified by |max_concurrent_queries_|. On 2016/10/03 23:51:08, Ryan Sleevi (slow) wrote: > Because you're asynchronously invoking this callback, this comment does not make > sense. Done.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robpercival@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2367523002/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Merge LogDnsClient's QueryLeafIndex and QueryAuditProof methods QueryAuditProof now takes a leaf hash, rather than a leaf index. It will look up the leaf index, then the audit proof. BUG=624894 ========== to ========== Merge LogDnsClient's QueryLeafIndex and QueryAuditProof methods QueryAuditProof now takes a leaf hash, rather than a leaf index. It will look up the leaf index, then the audit proof. BUG=624894 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Merge LogDnsClient's QueryLeafIndex and QueryAuditProof methods QueryAuditProof now takes a leaf hash, rather than a leaf index. It will look up the leaf index, then the audit proof. BUG=624894 ========== to ========== Merge LogDnsClient's QueryLeafIndex and QueryAuditProof methods QueryAuditProof now takes a leaf hash, rather than a leaf index. It will look up the leaf index, then the audit proof. BUG=624894 Committed: https://crrev.com/8da709c5cf63b5535f8c30ae6cc9e786bc8eaae8 Cr-Commit-Position: refs/heads/master@{#424758} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8da709c5cf63b5535f8c30ae6cc9e786bc8eaae8 Cr-Commit-Position: refs/heads/master@{#424758} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
