Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2066083002:
SafeBrowising: Read and write V4Store from/to disk (Closed)
|
Created:
4 years, 6 months ago by vakh (use Gerrit instead) Modified:
4 years, 5 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, noé, woz Base URL:
https://chromium.googlesource.com/chromium/src.git@01_UpdateDbAndStores Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRead and write V4Store from/to disk.
Stores the updates as a V4StoreFileFormat proto with the following fields:
1. Magic number (integer)
2. Version (integer)
3. Hash prefix information (ListUpdateResponse proto).
Overall design document: https://goto.google.com/design-doc-v4store
Only FULL_UPDATEs are written to disk.
Includes tests to read from and write to the disk.
BUG=543161
Committed: https://crrev.com/2d092db5bb51f021d7213076ad3b33590be6efc4
Cr-Commit-Position: refs/heads/master@{#402621}
Patch Set 1 #Patch Set 2 : Nit: Add some comments to the v4_store.proto file. #Patch Set 3 : Nit: better test names. Tests for WriteToDisk #
Total comments: 33
Patch Set 4 : git pull #Patch Set 5 : base::CreateAndOpenTemporaryFile doesn't work reliably on win_chromium_*rel_ng so using another mec… #Patch Set 6 : Incorporate some CR feedback. +Lint #Patch Set 7 : git pull from base branch: http://crrev.com/2062013002 #Patch Set 8 : git pull #Patch Set 9 : Track writes to store via UMA. Create a temp file for writing. Remove some DVLOGs. #
Total comments: 77
Patch Set 10 : git fetch && git pull && gclient sync #Patch Set 11 : CR feedback: shess@ (except tests) #Patch Set 12 : CR feedback: shess@ unittests #Patch Set 13 : Add a test for showing the importance of having magic number #Patch Set 14 : Update histograms.xml following the change in enum values in v4_store.h #Patch Set 15 : Nit: Remove an unused V4StoreFileFormat file_format; #
Total comments: 3
Patch Set 16 : CR feedback: shess@ latest comments #
Total comments: 5
Patch Set 17 : Minor: CR feedback from rkaplow@ #Patch Set 18 : git pull #
Total comments: 1
Patch Set 19 : Minor: Add units to histograms.xml #Patch Set 20 : Fix leak, use smart pointers #Patch Set 21 : Nit: Make store creation and use more consistent. #
Total comments: 1
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 54 (9 generated)
Nit: Add some comments to the v4_store.proto file.
vakh@chromium.org changed reviewers: + nparker@chromium.org, shess@chromium.org
nparker@, shess@: Please review the changes in * I'll update the CL description to be more descriptive soon.
Nit: better test names. Tests for WriteToDisk
https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:39: // If a state already exists, don't re-initilize. Would this indicate a bug? You could dcheck then. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:44: StoreReadFailureType store_read_failure_type = ReadFromDisk(); nit:How about StoreReadResult? FailureType doesn't fit when it's a success. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:108: return FILE_UNREADABLE_FAILURE; It would be good to differentiate "non existent" from other read failures. Otherwise, we'll have a guaranteed background rate of failures due to new installs. Could do it in a separate CL. We may be able to better than ReadFileToString, since it reads in chunks and expands the string incrementally, rather than reading / allocating in one block. Something to optimize later. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:129: file_format.version_number() < kFileVersion) { What does this protect against? It might be better to check != for now, and if we later want to increment it, we can decide for that specific version transition then how to handle older versions. We'd probably want an upgrade path, to avoid a sudden erase/reload of all clients. But for now, we should assume this can't read any other versions. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:132: return FILE_VERSION_TOO_LOW_FAILURE; Then this would be FILE_VERSION_INCOMPATIBLE https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:160: *response_to_write = response; This is going to make a copy (big). Instead, you could make |response| non-const and do response_to_write->swap(response). That'll empty |response|, which is probably fine. If not, you could swap it back after. Maybe there's a better pattern. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:164: size_t written = base::WriteFile(store_path_, file_format_string.c_str(), We should write to a tmp file in the same dir (maybe store_path_ + ".tmp"), and then do an atomic rename. Otherwise, any interruption in the writing will result in a bad file. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:166: return file_format_string.size() == written; TODO: Add UMA metrics: * File size * Time it takes to write, and read. Maybe it could be just one timing metric for reading all stores. * Write success https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.h:24: // Reserved for errors in parsing this enum. (if this isn't stored in a proto, it probably won't have a parse error, but this is a good general practice imho) https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.h:50: // file was fully written to disk. Or any general corruption on disk, ya? https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.proto (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.proto:16: // https://en.wikipedia.org/wiki/Magic_number_(programming) Is this to assert that your magic number hasn't been claimed? https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.proto:19: // Identifies the version of the file we are reading. // Identifies the version of the file format https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:32: V4Store* store = new V4Store(task_runner_, store_path_); Leaked ptr, here an below. Could just put these on the stack. Or even EXPECT_EQ(FILE_EMPTY_FAILURE, V4Store(task_runner_, store_path_).ReadFromDisk()) https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:139: EXPECT_EQ(SUCCESS, read_store->ReadFromDisk()); If you're going to test writing+reading you might as well test that some contents makes it through (e.g. to ensure that the swap() I suggested actually works). https://codereview.chromium.org/2066083002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2066083002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:46243: + Track the parsing results of reading the SafeBrowsing V4 store file from Say when/where this is recorded. e.g. "Recorded every time a store is read from disk" https://codereview.chromium.org/2066083002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:46250: + <summary>Version read from the store file.</summary> from a store file
git pull
base::CreateAndOpenTemporaryFile doesn't work reliably on win_chromium_*rel_ng so using another mechanism to create temp store files
Incorporate some CR feedback. +Lint
Some feedback incorporated, some left for later today. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:39: // If a state already exists, don't re-initilize. On 2016/06/16 00:07:50, Nathan Parker wrote: > Would this indicate a bug? You could dcheck then. Done. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:44: StoreReadFailureType store_read_failure_type = ReadFromDisk(); On 2016/06/16 00:07:50, Nathan Parker wrote: > nit:How about StoreReadResult? FailureType doesn't fit when it's a success. Done. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:108: return FILE_UNREADABLE_FAILURE; On 2016/06/16 00:07:50, Nathan Parker wrote: > It would be good to differentiate "non existent" from other read failures. > Otherwise, we'll have a guaranteed background rate of failures due to new > installs. Could do it in a separate CL. > > We may be able to better than ReadFileToString, since it reads in chunks and > expands the string incrementally, rather than reading / allocating in one block. > Something to optimize later. Acknowledged. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:129: file_format.version_number() < kFileVersion) { On 2016/06/16 00:07:50, Nathan Parker wrote: > What does this protect against? It provides the guarantee that we know we can parse some/all of this file. version 9 knows to look for new_client_state, additions, etc in ListUpdateResponse. version 10 could possibly look for other fields in ListUpdateResponse. > It might be better to check != for now, and if > we later want to increment it, we can decide for that specific version > transition then how to handle older versions. We'd probably want an upgrade > path, to avoid a sudden erase/reload of all clients. But for now, we should > assume this can't read any other versions. Fair enough. Done. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:132: return FILE_VERSION_TOO_LOW_FAILURE; On 2016/06/16 00:07:50, Nathan Parker wrote: > Then this would be FILE_VERSION_INCOMPATIBLE Done. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.h:24: // Reserved for errors in parsing this enum. On 2016/06/16 00:07:50, Nathan Parker wrote: > (if this isn't stored in a proto, it probably won't have a parse error, but this > is a good general practice imho) Acknowledged. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.h:50: // file was fully written to disk. On 2016/06/16 00:07:50, Nathan Parker wrote: > Or any general corruption on disk, ya? Done. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.proto (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.proto:16: // https://en.wikipedia.org/wiki/Magic_number_(programming) On 2016/06/16 00:07:50, Nathan Parker wrote: > Is this to assert that your magic number hasn't been claimed? To assert that the file being read is a Store file. Future versions of store may be very dissimilar in terms of the structure of the proto, but the magic signature should be the same. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.proto:19: // Identifies the version of the file we are reading. On 2016/06/16 00:07:50, Nathan Parker wrote: > // Identifies the version of the file format Done. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:32: V4Store* store = new V4Store(task_runner_, store_path_); On 2016/06/16 00:07:50, Nathan Parker wrote: > Leaked ptr, here an below. Could just put these on the stack. Or even > EXPECT_EQ(FILE_EMPTY_FAILURE, V4Store(task_runner_, store_path_).ReadFromDisk()) Done. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:139: EXPECT_EQ(SUCCESS, read_store->ReadFromDisk()); On 2016/06/16 00:07:50, Nathan Parker wrote: > If you're going to test writing+reading you might as well test that some > contents makes it through (e.g. to ensure that the swap() I suggested actually > works). Done. Verifying that the state read back from disk is as expected.
git pull from base branch: http://crrev.com/2062013002
git pull
Track writes to store via UMA. Create a temp file for writing. Remove some DVLOGs.
https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:164: size_t written = base::WriteFile(store_path_, file_format_string.c_str(), On 2016/06/16 00:07:50, Nathan Parker wrote: > We should write to a tmp file in the same dir (maybe store_path_ + ".tmp"), and > then do an atomic rename. Otherwise, any interruption in the writing will > result in a bad file. Done.
https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:55: // If a state already exists, don't re-initilize. Nit: remove this comment.
https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:58: DCHECK(base::CreateDirectory(base_path)); I don't think DCHECK and side effects mix well. Maybe I'm wrong. if(!CreateDir()) NOTREACHED(); maybe? [Or maybe you meant DCHECK(base::DirectoryExists(base_path))?] https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:18: // that byte-order changes force corruption. Protobuf would handle byte-ordering issues, so this wouldn't hold if the magic number is inside the protobuf envelope. OTOH, protobuf would handle byte-ordering issues, so it also doesn't need to flag byte-order changes. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:50: new_store->Initialize(); If this is only called from the factory, it might make sense to make the factory a friend and the method private. Or to friend and call ReadFromDisk() directly. OTOH, if it's paired with the constructor and the return value is void, why is it even broken out from the constructor? https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:55: // If a state already exists, don't re-initilize. On 2016/06/22 07:28:04, vakh wrote: > Nit: remove this comment. I'm fine with a comment, but the comment makes it sound like it's saying how the control flows, and what it's actually saying "Only ever initialize once." https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:74: store_path_.value().c_str(), state_base64.c_str()); Thanks! This can be base::StringPrintf() without |value|. I feel like I said that on a different review, so maybe this change broke a glass ceiling somewhere? https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:109: V4StoreFileFormat file_format; Push file_format down to where it's used. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:125: << file_format.magic_number(); What does this DVLOG do for !has_magic_number()? Are the cases of not having the right fields maybe something to rollup in a "wrong proto" case? Maybe this should be a DCHECK, I can't think of any cases where you'd actually expect something else here. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:130: file_format.version_number()); Should probably check if you have a version number before logging it. Unless it returns 0? If it returns 0, then don't even check if it has a version number, just check if the version number matches. Hmm, that applies to magic number, too. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:134: << "; expected: " << kFileVersion; Maybe this should be a DCHECK, I can't think of any cases where you'd actually expect something else here. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:158: DVLOG(1) << "response.response_type(): " << response.response_type(); I think just going with a DCHECK would be fair for these. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:170: *response_to_write = response; Is there no set_list_update_response() method? https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:174: file_format_string.size()); file_format_string.c_str() has to nul-terminate. You're passing size(), so maybe data() instead? https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:177: DVLOG(1) << "written: " << written; DCHECK_EQ() would seem reasonable, this should never fail. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:183: DVLOG(1) << "new_filename: " << new_filename.value(); new_filename can be derived (or store_path_ can be derived). So maybe switch to a single DVPLOG(1) statement to give error info. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.h:28: READ_SUCCESS = 1, There are many failures, but only one success - suggest putting success as 0. Same for writes. [Here and histograms.xml. Or ignore my OCD.] https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.h:111: void Initialize(); Seems like this must be something different from the constructor? Maybe it could use a comment. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.h:138: FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestWriteFullResponseType); I'd put these above the private functions. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.proto (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.proto:16: // https://en.wikipedia.org/wiki/Magic_number_(programming) It's a uint32 not a blob, so I think "magic number" is better than "magic bytes". Also, no need to be coy, you could just give the number, here, since it's mostly there just to make sure you aren't reading random garbage. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.proto:20: // Version 9 with PVer4 is very different from earlier versions. Hypothetical question: Does this even need a version number and magic number? The earlier code wanted a magic number for validation, and a version to determine how to read the rest of the file. Presumably the protobuf code already knows how to tell if it's reading a valid protobuf file, and once read you can use introspection to see what was contained. Hmm. If a v10 reader sees a v9 file, it can read it and then look to see if the file has the fields it is concerned with, so it doesn't need a version to do that. A version might be convenient, but the hope would be that the v10 reader doesn't even have to think about whether it's a v9 file, it just fills things in naturally. Going in reverse, a v9 reader should be able to read a v10 file, but it probably can't _write_ a v10 file, so it might strip information. But I don't think a v9 reader can recognize that it has a >v9 file without an explicit version number. That said, the magic number feels like something which belongs outside the envelope to validate the input, or dropped entirely if protobuf validates sufficiently. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.proto:24: // updates. For details about this message, see: safebrowsing.proto Drop "Contains the", that's implied by defining storage. Drop "being written" because that's also implied. The second sentence is degenerate, maybe ", see safebrowsing.proto." [Actually, you could also just drop the second sentence entirely. There's only one import, so it should be obvious where to look. Even if not, the symbols are long enough to be easy to search for.] https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:34: scoped_refptr<base::TestSimpleTaskRunner> task_runner_; Will task_runner_ destruction pump events? If so, might be reasonable to have task_runner_ after temp_dir_ in case outstanding tasks refer to tempfiles. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:41: { base::ScopedFILE new_file(base::OpenFile(store_path_, "wb+")); } I see what you're doing there, but I think the single-line scope isn't Chromium acceptable. How about just: base::CloseFile(base::OpenFile(...)); Maybe with a comment saying "Create an empty file"? Or, if you want to be tricksy: base::ScopedFILE(base::OpenFile(...)); But don't be tricksy! https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:50: } I like busting out all the cases for testing! https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:55: invalid_contents.size()); const kInvalidContents[] = "Chromium"; base::WriteFile(store_path_, kInvalidContents, ARRAY_SIZE(kInvalidContents)); Or strlen(). https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:67: file_format_string.size()); data() rather than c_str(). Right now I'm wishing for a WriteFile() which takes a StringPiece, to be honest. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:79: file_format_string.size()); data(). https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:91: file_format_string.size()); data(). Or an anonymous WriteStringToFile() helper. A WriteProtoToFile() helper would help even more. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:116: } Hmm. Now my DCHECK suggestions are looking wrong. I guess if you're going to test the error cases, you have to be able to complete the test. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:130: std::unique_ptr<V4Store> write_store(new V4Store(task_runner_, store_path_)); Could this just be a V4Store without the unique_ptr? https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:133: std::unique_ptr<V4Store> read_store(new V4Store(task_runner_, store_path_)); Also here?
https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.proto (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.proto:20: // Version 9 with PVer4 is very different from earlier versions. On 2016/06/22 22:58:39, Scott Hess wrote: > Hypothetical question: Does this even need a version number and magic number? > The earlier code wanted a magic number for validation, and a version to > determine how to read the rest of the file. Presumably the protobuf code > already knows how to tell if it's reading a valid protobuf file, and once read > you can use introspection to see what was contained. > > Hmm. If a v10 reader sees a v9 file, it can read it and then look to see if the > file has the fields it is concerned with, so it doesn't need a version to do > that. A version might be convenient, but the hope would be that the v10 reader > doesn't even have to think about whether it's a v9 file, it just fills things in > naturally. Going in reverse, a v9 reader should be able to read a v10 file, but > it probably can't _write_ a v10 file, so it might strip information. But I > don't think a v9 reader can recognize that it has a >v9 file without an explicit > version number. > > That said, the magic number feels like something which belongs outside the > envelope to validate the input, or dropped entirely if protobuf validates > sufficiently. I think the use for the version is if we want to upgrade the format, and we expect to be able to read both new and old. A v10 reader should be able to recognize/read a v9 file and write a v10 file. We should never need a v9 reader to read a v10 file (assuming the code doesn't get reverted in a subsequent stable release). The value in upgrading is so we don't have all stable clients dump their data and do a full update just because we want to change the on-disk format. We can also assume that the back-version reading code would need to live only for one release (those who are skip a release will just dump their data and do a full update). Magic number: This is useful if the protoparser does not validate sufficiently, such as if it silently ignores unparsable data at the end (and assumes they are fields from a newer version of the proto). If we can guarantee the protoparser has consumed all the data and understands it, then we don't need this.
git fetch && git pull && gclient sync
CR feedback: shess@ (except tests)
Addressed comments (except unittests file). PTAL. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:58: DCHECK(base::CreateDirectory(base_path)); On 2016/06/22 22:58:38, Scott Hess wrote: > I don't think DCHECK and side effects mix well. Maybe I'm wrong. It ensures that the directory exists or gets created. > if(!CreateDir()) NOTREACHED(); maybe? NOTREACHED == DCHECK(false); So it's exactly the same. > > [Or maybe you meant DCHECK(base::DirectoryExists(base_path))?] The directory may or may not exist already. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:18: // that byte-order changes force corruption. On 2016/06/22 22:58:38, Scott Hess wrote: > Protobuf would handle byte-ordering issues, so this wouldn't hold if the magic > number is inside the protobuf envelope. > > OTOH, protobuf would handle byte-ordering issues, so it also doesn't need to > flag byte-order changes. Removed the comment. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:50: new_store->Initialize(); On 2016/06/22 22:58:39, Scott Hess wrote: > If this is only called from the factory, it might make sense to make the factory > a friend and the method private. Or to friend and call ReadFromDisk() directly. > FakeV4StoreFactory, derived from V4StoreFactory, also calls this constructor so I'll need to make both of them friend. I like to make as few test friends as possible. Including in code ;-) > OTOH, if it's paired with the constructor and the return value is void, why is > it even broken out from the constructor? So that a test can create a V4Store object but can skip reading the disk file. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:74: store_path_.value().c_str(), state_base64.c_str()); On 2016/06/22 22:58:38, Scott Hess wrote: > Thanks! > > This can be base::StringPrintf() without |value|. I feel like I said that on a > different review, so maybe this change broke a glass ceiling somewhere? Done. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:109: V4StoreFileFormat file_format; On 2016/06/22 22:58:39, Scott Hess wrote: > Push file_format down to where it's used. Done. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:125: << file_format.magic_number(); On 2016/06/22 22:58:38, Scott Hess wrote: > What does this DVLOG do for !has_magic_number()? > You probably meant something else, but if you asking what would DVLOG print, it'd be an empty string. > Are the cases of not having the right fields maybe something to rollup in a > "wrong proto" case? > I think it'd be a useful metric to track: what exact error are we having while reading the store files. > Maybe this should be a DCHECK, I can't think of any cases where you'd actually > expect something else here. UMA tracking of common error cases. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:130: file_format.version_number()); On 2016/06/22 22:58:38, Scott Hess wrote: > Should probably check if you have a version number before logging it. Unless it > returns 0? > Yes, it returns 0 if it isn't set. We never expect to see version '0' so seeing this value in the logs would indicate that we found something totally unexpected. > If it returns 0, then don't even check if it has a version number, just check if > the version number matches. Hmm, that applies to magic number, too. Magic number is the first check and we expect exactly one value. If we get a wrong or unset magic number, then we are reading the wrong file. So that's a boolean case: right file or wrong file. If that check passes, then we are reading the right file. Now, if the version doesn't match our expectation, that's an interesting event. So we want to know what version was read, if any. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:134: << "; expected: " << kFileVersion; On 2016/06/22 22:58:38, Scott Hess wrote: > Maybe this should be a DCHECK, I can't think of any cases where you'd actually > expect something else here. DCHECKs make it hard to test conditions. I feel tests are a better way to enforce conditions, but I may be wrong. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:158: DVLOG(1) << "response.response_type(): " << response.response_type(); On 2016/06/22 22:58:39, Scott Hess wrote: > I think just going with a DCHECK would be fair for these. DCHECK vs tests, as above. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:170: *response_to_write = response; On 2016/06/22 22:58:38, Scott Hess wrote: > Is there no set_list_update_response() method? No, it is a repeated field so no set_X only add_X https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:174: file_format_string.size()); On 2016/06/22 22:58:38, Scott Hess wrote: > file_format_string.c_str() has to nul-terminate. You're passing size(), so > maybe data() instead? I didn't know about data(), but it looks like it's the same as c_str() From: http://www.cplusplus.com/reference/string/string/data/ Both string::data and string::c_str are synonyms and return the same value. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:177: DVLOG(1) << "written: " << written; On 2016/06/22 22:58:39, Scott Hess wrote: > DCHECK_EQ() would seem reasonable, this should never fail. Done. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:183: DVLOG(1) << "new_filename: " << new_filename.value(); On 2016/06/22 22:58:38, Scott Hess wrote: > new_filename can be derived (or store_path_ can be derived). So maybe switch to > a single DVPLOG(1) statement to give error info. Done. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.h:28: READ_SUCCESS = 1, On 2016/06/22 22:58:39, Scott Hess wrote: > There are many failures, but only one success - suggest putting success as 0. > Same for writes. > > [Here and histograms.xml. Or ignore my OCD.] It's a valid point. Done. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.h:111: void Initialize(); On 2016/06/22 22:58:39, Scott Hess wrote: > Seems like this must be something different from the constructor? Maybe it > could use a comment. Done. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.h:138: FRIEND_TEST_ALL_PREFIXES(V4StoreTest, TestWriteFullResponseType); On 2016/06/22 22:58:39, Scott Hess wrote: > I'd put these above the private functions. Done. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.proto (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.proto:16: // https://en.wikipedia.org/wiki/Magic_number_(programming) On 2016/06/22 22:58:39, Scott Hess wrote: > It's a uint32 not a blob, so I think "magic number" is better than "magic > bytes". Done. > Also, no need to be coy, you could just give the number, here, since > it's mostly there just to make sure you aren't reading random garbage. Not sure if that's really helpful as a comment. Magic numbers, by definition, are well defined. They can't be random garbage. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.proto:20: // Version 9 with PVer4 is very different from earlier versions. On 2016/06/23 00:37:55, Nathan Parker wrote: > On 2016/06/22 22:58:39, Scott Hess wrote: > > Hypothetical question: Does this even need a version number and magic number? > > The earlier code wanted a magic number for validation, and a version to > > determine how to read the rest of the file. Presumably the protobuf code > > already knows how to tell if it's reading a valid protobuf file, and once read > > you can use introspection to see what was contained. > > > > Hmm. If a v10 reader sees a v9 file, it can read it and then look to see if > the > > file has the fields it is concerned with, so it doesn't need a version to do > > that. A version might be convenient, but the hope would be that the v10 > reader > > doesn't even have to think about whether it's a v9 file, it just fills things > in > > naturally. Going in reverse, a v9 reader should be able to read a v10 file, > but > > it probably can't _write_ a v10 file, so it might strip information. But I > > don't think a v9 reader can recognize that it has a >v9 file without an > explicit > > version number. > > > > That said, the magic number feels like something which belongs outside the > > envelope to validate the input, or dropped entirely if protobuf validates > > sufficiently. > > I think the use for the version is if we want to upgrade the format, and we > expect to be able to read both new and old. A v10 reader should be able to > recognize/read a v9 file and write a v10 file. We should never need a v9 reader > to read a v10 file (assuming the code doesn't get reverted in a subsequent > stable release). The value in upgrading is so we don't have all stable clients > dump their data and do a full update just because we want to change the on-disk > format. We can also assume that the back-version reading code would need to > live only for one release (those who are skip a release will just dump their > data and do a full update). > > Magic number: This is useful if the protoparser does not validate sufficiently, > such as if it silently ignores unparsable data at the end (and assumes they are > fields from a newer version of the proto). If we can guarantee the protoparser > has consumed all the data and understands it, then we don't need this. The goal of the magic number is to ensure that we are reading a store file. Just being able to parse a proto doesn't ensure that. I am going to add a test to demonstrate that. The goal of version is: Now that we know we are reading a store file, what should we expect to be set and also helps us track what version files being used in the world. Examples: 1. How many users are on v9 vs v10.. 2. Are there any users reading store files but some how getting unexpected version numbers? 3. Since we know this is version 9, we might expect some fields to be set and others to be ignored. This allows us to deal better with breaking changes down the line. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.proto:24: // updates. For details about this message, see: safebrowsing.proto On 2016/06/22 22:58:39, Scott Hess wrote: > Drop "Contains the", that's implied by defining storage. Drop "being written" > because that's also implied. The second sentence is degenerate, maybe ", see > safebrowsing.proto." > > [Actually, you could also just drop the second sentence entirely. There's only > one import, so it should be obvious where to look. Even if not, the symbols are > long enough to be easy to search for.] Done. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:116: } On 2016/06/22 22:58:40, Scott Hess wrote: > Hmm. Now my DCHECK suggestions are looking wrong. I guess if you're going to > test the error cases, you have to be able to complete the test. Acknowledged.
CR feedback: shess@ unittests
Add a test for showing the importance of having magic number
And now I've addressed the comments in the unit tests. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:34: scoped_refptr<base::TestSimpleTaskRunner> task_runner_; On 2016/06/22 22:58:39, Scott Hess wrote: > Will task_runner_ destruction pump events? If so, might be reasonable to have > task_runner_ after temp_dir_ in case outstanding tasks refer to tempfiles. Thanks! Didn't think about that. The detail with which you do code reviews is mind-boggling. Thanks a lot! https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:41: { base::ScopedFILE new_file(base::OpenFile(store_path_, "wb+")); } On 2016/06/22 22:58:40, Scott Hess wrote: > I see what you're doing there, but I think the single-line scope isn't Chromium > acceptable. > > How about just: > base::CloseFile(base::OpenFile(...)); > Maybe with a comment saying "Create an empty file"? > > Or, if you want to be tricksy: > base::ScopedFILE(base::OpenFile(...)); > But don't be tricksy! This formatting was done by "git cl format". Doesn't that make it acceptable? :) https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:50: } On 2016/06/22 22:58:39, Scott Hess wrote: > I like busting out all the cases for testing! Acknowledged. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:55: invalid_contents.size()); On 2016/06/22 22:58:40, Scott Hess wrote: > const kInvalidContents[] = "Chromium"; > base::WriteFile(store_path_, kInvalidContents, ARRAY_SIZE(kInvalidContents)); > > Or strlen(). Done. Fits on a line too! https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:67: file_format_string.size()); On 2016/06/22 22:58:39, Scott Hess wrote: > data() rather than c_str(). > > Right now I'm wishing for a WriteFile() which takes a StringPiece, to be honest. It's the same. data and c_str https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:79: file_format_string.size()); On 2016/06/22 22:58:39, Scott Hess wrote: > data(). See above. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:91: file_format_string.size()); On 2016/06/22 22:58:39, Scott Hess wrote: > data(). Or an anonymous WriteStringToFile() helper. A WriteProtoToFile() > helper would help even more. Done. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:130: std::unique_ptr<V4Store> write_store(new V4Store(task_runner_, store_path_)); On 2016/06/22 22:58:39, Scott Hess wrote: > Could this just be a V4Store without the unique_ptr? Done. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:133: std::unique_ptr<V4Store> read_store(new V4Store(task_runner_, store_path_)); On 2016/06/22 22:58:40, Scott Hess wrote: > Also here? Done.
Update histograms.xml following the change in enum values in v4_store.h
Nit: Remove an unused V4StoreFileFormat file_format;
https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:160: *response_to_write = response; On 2016/06/16 00:07:50, Nathan Parker wrote: > This is going to make a copy (big). Instead, you could make |response| > non-const and do response_to_write->swap(response). That'll empty |response|, > which is probably fine. If not, you could swap it back after. Maybe there's a > better pattern. Acknowledged. I am going to try that as a separate CL since it requires function signatures over multiple files so it's probably better done as a separate independent change. Sounds good?
Comments on comments. Will follow with comments-on-code. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:58: DCHECK(base::CreateDirectory(base_path)); On 2016/06/23 23:09:31, vakh wrote: > On 2016/06/22 22:58:38, Scott Hess wrote: > > I don't think DCHECK and side effects mix well. Maybe I'm wrong. > It ensures that the directory exists or gets created. > > > if(!CreateDir()) NOTREACHED(); maybe? > NOTREACHED == DCHECK(false); > So it's exactly the same. > > > > > [Or maybe you meant DCHECK(base::DirectoryExists(base_path))?] > The directory may or may not exist already. What I'm saying is that (I think) you want the CreateDirectory() to run in all cases, even in production, but in production it may be compiled away (see definition of DCHECK). So if it's like: if (!base::CreateDirectory(...)) NOTREACHED(); then the CreateDirectory() happens in debug or production, but the NOTREACHED() only happens in debug, in production the code will just continue. [I'm not 100% certain of this - this _is_ a problem with assert(), though, and IMHO you shouldn't venture into the subtle differences between assert() and DCHECK() if you can help it.] https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:50: new_store->Initialize(); On 2016/06/23 23:09:32, vakh wrote: > On 2016/06/22 22:58:39, Scott Hess wrote: > > If this is only called from the factory, it might make sense to make the > factory > > a friend and the method private. Or to friend and call ReadFromDisk() > directly. > > > FakeV4StoreFactory, derived from V4StoreFactory, also calls this constructor so > I'll need to make both of them friend. > I like to make as few test friends as possible. Including in code ;-) > > > OTOH, if it's paired with the constructor and the return value is void, why is > > it even broken out from the constructor? > So that a test can create a V4Store object but can skip reading the disk file. Acknowledged. But it kind of feels like your inheritence is upside down, in that case. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:125: << file_format.magic_number(); On 2016/06/23 23:09:31, vakh wrote: > On 2016/06/22 22:58:38, Scott Hess wrote: > > What does this DVLOG do for !has_magic_number()? > > > You probably meant something else, but if you asking what would DVLOG print, > it'd be an empty string. Mostly I meant that this case is both !has_magic_number() and magic_number() is wrong, and if !has_magic_number() holds I would assume that calling magic_number() may be undefined. Flipping that upside-down - if magic_number() is well-defined when !has_magic_number(), then why bother to check has_magic_number() at all? The well-defined value will presumably be != kFileMagic, which simplifies the condition. > > Are the cases of not having the right fields maybe something to rollup in a > > "wrong proto" case? > > > I think it'd be a useful metric to track: what exact error are we having while > reading the store files. I suspect that you'll either get an entirely-valid file, or an entirely-invalid file. > > Maybe this should be a DCHECK, I can't think of any cases where you'd actually > > expect something else here. > UMA tracking of common error cases. I meant the DVLOG() could be a DCHECK(). In production, you'll get the UMA stats, but it's unclear why you'd ever have an invalid file on your development machine. EXCEPT that you're doing testing, so you can't be DCHECK'ing in here, so answered. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:134: << "; expected: " << kFileVersion; On 2016/06/23 23:09:31, vakh wrote: > On 2016/06/22 22:58:38, Scott Hess wrote: > > Maybe this should be a DCHECK, I can't think of any cases where you'd actually > > expect something else here. > > DCHECKs make it hard to test conditions. I feel tests are a better way to > enforce conditions, but I may be wrong. Acknowledged. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:170: *response_to_write = response; On 2016/06/23 23:09:31, vakh wrote: > On 2016/06/22 22:58:38, Scott Hess wrote: > > Is there no set_list_update_response() method? > > No, it is a repeated field so no set_X only add_X Acknowledged. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:174: file_format_string.size()); On 2016/06/23 23:09:32, vakh wrote: > On 2016/06/22 22:58:38, Scott Hess wrote: > > file_format_string.c_str() has to nul-terminate. You're passing size(), so > > maybe data() instead? > > I didn't know about data(), but it looks like it's the same as c_str() > From: http://www.cplusplus.com/reference/string/string/data/ > Both string::data and string::c_str are synonyms and return the same value. Sigh. This changed between C++98 and C++11. I think I'd still prefer data(), because you're not interested in a c-string (nul characters aren't special), so it more closely describes what you're doing. But I think there's room for preference here. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.proto (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.proto:16: // https://en.wikipedia.org/wiki/Magic_number_(programming) On 2016/06/23 23:09:32, vakh wrote: > On 2016/06/22 22:58:39, Scott Hess wrote: > > Also, no need to be coy, you could just give the number, here, since > > it's mostly there just to make sure you aren't reading random garbage. > Not sure if that's really helpful as a comment. > Magic numbers, by definition, are well defined. They can't be random garbage. Well, technically the number read here _could_ be random garbage which just happens to be the magic number. Mostly my point is that if this could be considered documentation for the file format, then "There's an integer 123456789 here" makes sense, rather than "Here is a magic number which is defined somewhere else". It could even be defined in this file as an enum. I suppose it can't be the default for the field, though :-). https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.proto:20: // Version 9 with PVer4 is very different from earlier versions. On 2016/06/23 23:09:32, vakh wrote: > The goal of the magic number is to ensure that we are reading a store file. > Just being able to parse a proto doesn't ensure that. I am going to add a test > to demonstrate that. With the earlier raw file, the magic sufficed to differentiate a SQLite file from a raw file, as they have different magic in the first few bytes. But that number was logically checked _before_ the other data was read. In this case, we already know you're reading a valid-ish protobuf. The magic number would add more certainty, I guess... but does it add more certainly over just a happy version number? [It's cheap to put in there, mostly I'm just trying to be extra skeptical at the creation, since once in it stays forever.] > The goal of version is: Now that we know we are reading a store file, what > should we expect to be set and also helps us track what version files being used > in the world. > Examples: > 1. How many users are on v9 vs v10.. > 2. Are there any users reading store files but some how getting unexpected > version numbers? > 3. Since we know this is version 9, we might expect some fields to be set and > others to be ignored. This allows us to deal better with breaking changes down > the line. #1 seems legit to me, it would be weird to reverse-engineer a version indicator from field presence. #2 seems like a stretch, I'm not sure it matters in the end. #3 is where I can be convinced, but as-is I'm not entirely sure. The problem as I see it is that the kinds of notably-breaking changes you're suggesting also imply that you're going to end up with a lot of unused optional fields. Like you'll add a bunch of new fields which replace the original fields, and your code will say "For version 10, don't see all these fields, do set all those fields". To me, that feels like you really want to just have a different protobuf. Which is why I mentioned "envelope". Like if you had a header message which contained magic+version, then used the version to drive selection of which kind of message to read next. Or is this implicit? Like you might just have messages always defined with fields 1 and 2 a particular way, and the v9 message might have different fields than the v10 message beyond that? That seems brittle, to me. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:41: { base::ScopedFILE new_file(base::OpenFile(store_path_, "wb+")); } On 2016/06/23 23:36:21, vakh wrote: > On 2016/06/22 22:58:40, Scott Hess wrote: > > I see what you're doing there, but I think the single-line scope isn't > Chromium > > acceptable. > > > > How about just: > > base::CloseFile(base::OpenFile(...)); > > Maybe with a comment saying "Create an empty file"? > > > > Or, if you want to be tricksy: > > base::ScopedFILE(base::OpenFile(...)); > > But don't be tricksy! > > This formatting was done by "git cl format". Doesn't that make it acceptable? :) Really? I think that's the first time I've ever seen a single-line non-empty block. Still, I think CloseFile(OpenFile(...)) may be more obvious. I'll leave that to your discretion.
Guess it was mostly comments on comments, then. https://codereview.chromium.org/2066083002/diff/270001/components/safe_browsi... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2066083002/diff/270001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:36: ListUpdateResponse* response = nullptr) { Are the default parameters allowed by the style guide? If it is, I think it's alright. But if not, I was thinking more along the lines of WriteProtoToFile(const V4StoreFileFomat& file_format), like the caller makes the proto, but all of the "convert to string, string to file" boilerplate consolidated in one place.
CR feedback: shess@ latest comments
https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:58: DCHECK(base::CreateDirectory(base_path)); On 2016/06/24 04:14:11, Scott Hess wrote: > On 2016/06/23 23:09:31, vakh wrote: > > On 2016/06/22 22:58:38, Scott Hess wrote: > > > I don't think DCHECK and side effects mix well. Maybe I'm wrong. > > It ensures that the directory exists or gets created. > > > > > if(!CreateDir()) NOTREACHED(); maybe? > > NOTREACHED == DCHECK(false); > > So it's exactly the same. > > > > > > > > [Or maybe you meant DCHECK(base::DirectoryExists(base_path))?] > > The directory may or may not exist already. > > What I'm saying is that (I think) you want the CreateDirectory() to run in all > cases, even in production, but in production it may be compiled away (see > definition of DCHECK). So if it's like: > if (!base::CreateDirectory(...)) > NOTREACHED(); > then the CreateDirectory() happens in debug or production, but the NOTREACHED() > only happens in debug, in production the code will just continue. > Yes, didn't realize that. Thanks for catching. Done. > [I'm not 100% certain of this - this _is_ a problem with assert(), though, and > IMHO you shouldn't venture into the subtle differences between assert() and > DCHECK() if you can help it.] I agree. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:55: // If a state already exists, don't re-initilize. On 2016/06/22 22:58:38, Scott Hess wrote: > On 2016/06/22 07:28:04, vakh wrote: > > Nit: remove this comment. > > I'm fine with a comment, but the comment makes it sound like it's saying how the > control flows, and what it's actually saying "Only ever initialize once." Acknowledged. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:125: << file_format.magic_number(); On 2016/06/24 04:14:11, Scott Hess wrote: > On 2016/06/23 23:09:31, vakh wrote: > > On 2016/06/22 22:58:38, Scott Hess wrote: > > > What does this DVLOG do for !has_magic_number()? > > > > > You probably meant something else, but if you asking what would DVLOG print, > > it'd be an empty string. > > Mostly I meant that this case is both !has_magic_number() and magic_number() is > wrong, and if !has_magic_number() holds I would assume that calling > magic_number() may be undefined. > > Flipping that upside-down - if magic_number() is well-defined when > !has_magic_number(), then why bother to check has_magic_number() at all? The > well-defined value will presumably be != kFileMagic, which simplifies the > condition. > Done. > > > Are the cases of not having the right fields maybe something to rollup in a > > > "wrong proto" case? > > > > > I think it'd be a useful metric to track: what exact error are we having while > > reading the store files. > > I suspect that you'll either get an entirely-valid file, or an entirely-invalid > file. > Yes, that's my expectation also, but I think it is better to be explicit about this. > > > Maybe this should be a DCHECK, I can't think of any cases where you'd > actually > > > expect something else here. > > UMA tracking of common error cases. > > I meant the DVLOG() could be a DCHECK(). In production, you'll get the UMA > stats, but it's unclear why you'd ever have an invalid file on your development > machine. EXCEPT that you're doing testing, so you can't be DCHECK'ing in here, > so answered. Ack. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:174: file_format_string.size()); On 2016/06/24 04:14:11, Scott Hess wrote: > On 2016/06/23 23:09:32, vakh wrote: > > On 2016/06/22 22:58:38, Scott Hess wrote: > > > file_format_string.c_str() has to nul-terminate. You're passing size(), so > > > maybe data() instead? > > > > I didn't know about data(), but it looks like it's the same as c_str() > > From: http://www.cplusplus.com/reference/string/string/data/ > > Both string::data and string::c_str are synonyms and return the same value. > > Sigh. This changed between C++98 and C++11. > > I think I'd still prefer data(), because you're not interested in a c-string > (nul characters aren't special), so it more closely describes what you're doing. > But I think there's room for preference here. Done. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store.proto (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.proto:16: // https://en.wikipedia.org/wiki/Magic_number_(programming) On 2016/06/24 04:14:11, Scott Hess wrote: > On 2016/06/23 23:09:32, vakh wrote: > > On 2016/06/22 22:58:39, Scott Hess wrote: > > > Also, no need to be coy, you could just give the number, here, since > > > it's mostly there just to make sure you aren't reading random garbage. > > Not sure if that's really helpful as a comment. > > Magic numbers, by definition, are well defined. They can't be random garbage. > > Well, technically the number read here _could_ be random garbage which just > happens to be the magic number. > If I come across a random file that happens to have a proto with this magic number in front, I'd bet my money that this was a real store file (or intended to be one) but somehow something went wrong in writing it. > Mostly my point is that if this could be considered documentation for the file > format, then "There's an integer 123456789 here" makes sense, rather than "Here > is a magic number which is defined somewhere else". It could even be defined in > this file as an enum. I suppose it can't be the default for the field, though > :-). Fair enough. Done. Unfortunately, can't enforce a default value. Also, having a magic number as an enum sounds principally wrong. https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store.proto:20: // Version 9 with PVer4 is very different from earlier versions. On 2016/06/24 04:14:11, Scott Hess wrote: > On 2016/06/23 23:09:32, vakh wrote: > > The goal of the magic number is to ensure that we are reading a store file. > > Just being able to parse a proto doesn't ensure that. I am going to add a test > > to demonstrate that. > > With the earlier raw file, the magic sufficed to differentiate a SQLite file > from a raw file, as they have different magic in the first few bytes. But that > number was logically checked _before_ the other data was read. In this case, we > already know you're reading a valid-ish protobuf. The magic number would add > more certainty, I guess... but does it add more certainly over just a happy > version number? > A version number is just an integer whose value we can't assert. If we skip the magic number, we'll be accepting any file that was a proto with the first field as an integer, which is arguably the most common first field in protos. The magic number and version number serve completely different purposes and I don't think we can overload either. > [It's cheap to put in there, mostly I'm just trying to be extra skeptical at the > creation, since once in it stays forever.] > Yes, I agree it is good to have this discussion. > > The goal of version is: Now that we know we are reading a store file, what > > should we expect to be set and also helps us track what version files being > used > > in the world. > > Examples: > > 1. How many users are on v9 vs v10.. > > 2. Are there any users reading store files but some how getting unexpected > > version numbers? > > 3. Since we know this is version 9, we might expect some fields to be set and > > others to be ignored. This allows us to deal better with breaking changes down > > the line. > > #1 seems legit to me, it would be weird to reverse-engineer a version indicator > from field presence. #2 seems like a stretch, I'm not sure it matters in the > end. #3 is where I can be convinced, but as-is I'm not entirely sure. > > The problem as I see it is that the kinds of notably-breaking changes you're > suggesting also imply that you're going to end up with a lot of unused optional > fields. This is fine and acceptable with protos. If the field is optional, it is allowed to be unused and if it is unused, it doesn't take any space on the disk. > Like you'll add a bunch of new fields which replace the original > fields, You probably know this, but the recommendation for doing that with protos is to add additional fields, not replace the existing ones. The existing ones, if unused, don't get serialized. > and your code will say "For version 10, don't see all these fields, do > set all those fields". To me, that feels like you really want to just have a > different protobuf. Which is why I mentioned "envelope". Like if you had a > header message which contained magic+version, then used the version to drive > selection of which kind of message to read next. > > Or is this implicit? Like you might just have messages always defined with > fields 1 and 2 a particular way, and the v9 message might have different fields > than the v10 message beyond that? That seems brittle, to me. To be frank, by using the has_* methods defined on protobufs, it is possible to parse any version without knowing the exact version number. What I meant is that can be done using the version number also. Also, since the ListUpdateResponse proto isn't strictly defined in Chromium itself (imported from SafeBrowsing PVer4 service's definition in google3), I think it is useful to have a way to check version number in case they make breaking changes to the proto (which I trust them not to, but would also like to protect us against). https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2066083002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:41: { base::ScopedFILE new_file(base::OpenFile(store_path_, "wb+")); } On 2016/06/24 04:14:11, Scott Hess wrote: > On 2016/06/23 23:36:21, vakh wrote: > > On 2016/06/22 22:58:40, Scott Hess wrote: > > > I see what you're doing there, but I think the single-line scope isn't > > Chromium > > > acceptable. > > > > > > How about just: > > > base::CloseFile(base::OpenFile(...)); > > > Maybe with a comment saying "Create an empty file"? > > > > > > Or, if you want to be tricksy: > > > base::ScopedFILE(base::OpenFile(...)); > > > But don't be tricksy! > > > > This formatting was done by "git cl format". Doesn't that make it acceptable? > :) > > Really? I think that's the first time I've ever seen a single-line non-empty > block. > > Still, I think CloseFile(OpenFile(...)) may be more obvious. I'll leave that to > your discretion. Done. https://codereview.chromium.org/2066083002/diff/270001/components/safe_browsi... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2066083002/diff/270001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:36: ListUpdateResponse* response = nullptr) { On 2016/06/24 04:20:33, Scott Hess wrote: > Are the default parameters allowed by the style guide? If it is, I think it's > alright. But if not, I was thinking more along the lines of > WriteProtoToFile(const V4StoreFileFomat& file_format), like the caller makes the > proto, but all of the "convert to string, string to file" boilerplate > consolidated in one place. Not forbidden here: https://chromium-cpp.appspot.com/ Allowed here: http://go/cpp-style#Default_Arguments
vakh@chromium.org changed reviewers: + rkaplow@chromium.org
I feel like we are close to done here for code changes so adding rkaplow@ now. rkaplow@: Could you please review the changes in histograms.xml?
Description was changed from ========== Read and write V4Store from/to disk. With tests for Read. BUG=543161 ========== to ========== Read and write V4Store from/to disk. Stores the updates as a V4StoreFileFormat proto with the following fields: 1. Magic number (integer) 2. Version (integer) 3. Hash prefix information (ListUpdateResponse proto). Only FULL_UPDATEs are written to disk. Includes tests to read from and write to the disk. BUG=543161 ==========
lgtm, I think. AFAICT there's nothing outstanding. https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:160: *response_to_write = response; On 2016/06/24 01:04:49, vakh wrote: > On 2016/06/16 00:07:50, Nathan Parker wrote: > > This is going to make a copy (big). Instead, you could make |response| > > non-const and do response_to_write->swap(response). That'll empty |response|, > > which is probably fine. If not, you could swap it back after. Maybe there's a > > better pattern. > > Acknowledged. I am going to try that as a separate CL since it requires function > signatures over multiple files so it's probably better done as a separate > independent change. Sounds good? I'd say to look at this comprehensively, starting from passing ownership into V4Database::ApplyUpdate(), which could take ownership of the vector then parcel it out into individual responses (or post the entire thing to the background as I suggested elsewhere), which in turn pass ownership into the store's ApplyUpdate(), perhaps using unique_ptr<> to manage ownership, etc. That said, the step merging the response into the existing data is probably where the majority of the memory overhead will be in the long-term, once the database has reached a steady state. https://codereview.chromium.org/2066083002/diff/270001/components/safe_browsi... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2066083002/diff/270001/components/safe_browsi... components/safe_browsing_db/v4_store_unittest.cc:36: ListUpdateResponse* response = nullptr) { On 2016/06/24 18:03:50, vakh wrote: > On 2016/06/24 04:20:33, Scott Hess wrote: > > Are the default parameters allowed by the style guide? If it is, I think it's > > alright. But if not, I was thinking more along the lines of > > WriteProtoToFile(const V4StoreFileFomat& file_format), like the caller makes > the > > proto, but all of the "convert to string, string to file" boilerplate > > consolidated in one place. > > Not forbidden here: > https://chromium-cpp.appspot.com/ > > Allowed here: > http://go/cpp-style#Default_Arguments Acknowledged. I think this must have changed at some point, I'm pretty sure it used to be "Avoid unless there's just no decent alternative."
https://codereview.chromium.org/2066083002/diff/290001/components/safe_browsi... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/290001/components/safe_browsi... components/safe_browsing_db/v4_store.cc:8: #include "base/metrics/histogram.h" histogram_macros.h https://codereview.chromium.org/2066083002/diff/290001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2066083002/diff/290001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46488: +<histogram name="SafeBrowsing.V4StoreVersionRead"> unit? https://codereview.chromium.org/2066083002/diff/290001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46490: + <summary>Version read from a store file.</summary> also this seems a bit overly terse, not sure what you mean
Minor: CR feedback from rkaplow@
Thanks for the review, rkaplow@ -- PTAL. https://codereview.chromium.org/2066083002/diff/290001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2066083002/diff/290001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46488: +<histogram name="SafeBrowsing.V4StoreVersionRead"> On 2016/06/27 17:48:51, rkaplow wrote: > unit? This is just a number, it doesn't have any units. For example, version 9 or 10.. https://codereview.chromium.org/2066083002/diff/290001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46490: + <summary>Version read from a store file.</summary> On 2016/06/27 17:48:51, rkaplow wrote: > also this seems a bit overly terse, not sure what you mean Done.
https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:160: *response_to_write = response; On 2016/06/24 19:51:17, Scott Hess wrote: > On 2016/06/24 01:04:49, vakh wrote: > > On 2016/06/16 00:07:50, Nathan Parker wrote: > > > This is going to make a copy (big). Instead, you could make |response| > > > non-const and do response_to_write->swap(response). That'll empty > |response|, > > > which is probably fine. If not, you could swap it back after. Maybe there's > a > > > better pattern. > > > > Acknowledged. I am going to try that as a separate CL since it requires > function > > signatures over multiple files so it's probably better done as a separate > > independent change. Sounds good? > > I'd say to look at this comprehensively, starting from passing ownership into > V4Database::ApplyUpdate(), which could take ownership of the vector then parcel > it out into individual responses (or post the entire thing to the background as > I suggested elsewhere), which in turn pass ownership into the store's > ApplyUpdate(), perhaps using unique_ptr<> to manage ownership, etc. > > That said, the step merging the response into the existing data is probably > where the majority of the memory overhead will be in the long-term, once the > database has reached a steady state. I'm working on this. CL coming today (or tomorrow, some tests failing).
https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:160: *response_to_write = response; On 2016/06/27 19:54:21, vakh wrote: > On 2016/06/24 19:51:17, Scott Hess wrote: > > On 2016/06/24 01:04:49, vakh wrote: > > > On 2016/06/16 00:07:50, Nathan Parker wrote: > > > > This is going to make a copy (big). Instead, you could make |response| > > > > non-const and do response_to_write->swap(response). That'll empty > > |response|, > > > > which is probably fine. If not, you could swap it back after. Maybe > there's > > a > > > > better pattern. > > > > > > Acknowledged. I am going to try that as a separate CL since it requires > > function > > > signatures over multiple files so it's probably better done as a separate > > > independent change. Sounds good? > > > > I'd say to look at this comprehensively, starting from passing ownership into > > V4Database::ApplyUpdate(), which could take ownership of the vector then > parcel > > it out into individual responses (or post the entire thing to the background > as > > I suggested elsewhere), which in turn pass ownership into the store's > > ApplyUpdate(), perhaps using unique_ptr<> to manage ownership, etc. > > > > That said, the step merging the response into the existing data is probably > > where the majority of the memory overhead will be in the long-term, once the > > database has reached a steady state. > > I'm working on this. CL coming today (or tomorrow, some tests failing). https://codereview.chromium.org/2103693002/ https://codereview.chromium.org/2066083002/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:160: *response_to_write = response; On 2016/06/27 19:54:21, vakh wrote: > On 2016/06/24 19:51:17, Scott Hess wrote: > > On 2016/06/24 01:04:49, vakh wrote: > > > On 2016/06/16 00:07:50, Nathan Parker wrote: > > > > This is going to make a copy (big). Instead, you could make |response| > > > > non-const and do response_to_write->swap(response). That'll empty > > |response|, > > > > which is probably fine. If not, you could swap it back after. Maybe > there's > > a > > > > better pattern. > > > > > > Acknowledged. I am going to try that as a separate CL since it requires > > function > > > signatures over multiple files so it's probably better done as a separate > > > independent change. Sounds good? > > > > I'd say to look at this comprehensively, starting from passing ownership into > > V4Database::ApplyUpdate(), which could take ownership of the vector then > parcel > > it out into individual responses (or post the entire thing to the background > as > > I suggested elsewhere), which in turn pass ownership into the store's > > ApplyUpdate(), perhaps using unique_ptr<> to manage ownership, etc. > > > > That said, the step merging the response into the existing data is probably > > where the majority of the memory overhead will be in the long-term, once the > > database has reached a steady state. > > I'm working on this. CL coming today (or tomorrow, some tests failing). Here's the CL: https://codereview.chromium.org/2103693002/ That one's also ready for review now.
Description was changed from ========== Read and write V4Store from/to disk. Stores the updates as a V4StoreFileFormat proto with the following fields: 1. Magic number (integer) 2. Version (integer) 3. Hash prefix information (ListUpdateResponse proto). Only FULL_UPDATEs are written to disk. Includes tests to read from and write to the disk. BUG=543161 ========== to ========== Read and write V4Store from/to disk. Stores the updates as a V4StoreFileFormat proto with the following fields: 1. Magic number (integer) 2. Version (integer) 3. Hash prefix information (ListUpdateResponse proto). Overall design document: https://goto.google.com/design-doc-v4store Only FULL_UPDATEs are written to disk. Includes tests to read from and write to the disk. BUG=543161 ==========
git pull
lgtm https://codereview.chromium.org/2066083002/diff/330001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2066083002/diff/330001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46639: +<histogram name="SafeBrowsing.V4StoreVersionRead"> we still always want either a unit or enum. I would just put unit="version number" then
Minor: Add units to histograms.xml
On 2016/06/28 14:42:15, rkaplow wrote: > lgtm > > https://codereview.chromium.org/2066083002/diff/330001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2066083002/diff/330001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:46639: +<histogram > name="SafeBrowsing.V4StoreVersionRead"> > we still always want either a unit or enum. I would just put unit="version > number" then Done.
Fix leak, use smart pointers
Nit: Make store creation and use more consistent.
The CQ bit was checked by vakh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2066083002/#ps390001 (title: "Nit: Make store creation and use more consistent.")
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 ========== Read and write V4Store from/to disk. Stores the updates as a V4StoreFileFormat proto with the following fields: 1. Magic number (integer) 2. Version (integer) 3. Hash prefix information (ListUpdateResponse proto). Overall design document: https://goto.google.com/design-doc-v4store Only FULL_UPDATEs are written to disk. Includes tests to read from and write to the disk. BUG=543161 ========== to ========== Read and write V4Store from/to disk. Stores the updates as a V4StoreFileFormat proto with the following fields: 1. Magic number (integer) 2. Version (integer) 3. Hash prefix information (ListUpdateResponse proto). Overall design document: https://goto.google.com/design-doc-v4store Only FULL_UPDATEs are written to disk. Includes tests to read from and write to the disk. BUG=543161 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:390001)
Message was sent while issue was closed.
Description was changed from ========== Read and write V4Store from/to disk. Stores the updates as a V4StoreFileFormat proto with the following fields: 1. Magic number (integer) 2. Version (integer) 3. Hash prefix information (ListUpdateResponse proto). Overall design document: https://goto.google.com/design-doc-v4store Only FULL_UPDATEs are written to disk. Includes tests to read from and write to the disk. BUG=543161 ========== to ========== Read and write V4Store from/to disk. Stores the updates as a V4StoreFileFormat proto with the following fields: 1. Magic number (integer) 2. Version (integer) 3. Hash prefix information (ListUpdateResponse proto). Overall design document: https://goto.google.com/design-doc-v4store Only FULL_UPDATEs are written to disk. Includes tests to read from and write to the disk. BUG=543161 Committed: https://crrev.com/2d092db5bb51f021d7213076ad3b33590be6efc4 Cr-Commit-Position: refs/heads/master@{#402621} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/2d092db5bb51f021d7213076ad3b33590be6efc4 Cr-Commit-Position: refs/heads/master@{#402621}
Message was sent while issue was closed.
horo@chromium.org changed reviewers: + horo@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2066083002/diff/390001/components/safe_browsi... File components/safe_browsing_db.gypi (right): https://codereview.chromium.org/2066083002/diff/390001/components/safe_browsi... components/safe_browsing_db.gypi:114: 'target_name': 'v4_store_proto', need dependencies to safebrowsing_proto https://codereview.chromium.org/2106323002/ |
