|
|
DescriptionAdd new SystemLogsSource type: SingleLogSource
This patch adds a new type of SystemLogsSource, SingleLogSource, which
provides a single log source in contrast to the many log sources provided
by the other implementations of SystemLogsSource.
The SingleLogSource currently supports incremental read of a limited set
of log files. It can be expanded to include other log files and other log
sources.
BUG=chromium:715263
Review-Url: https://codereview.chromium.org/2844163005
Cr-Commit-Position: refs/heads/master@{#471353}
Committed: https://chromium.googlesource.com/chromium/src/+/def38083bf08ec479bcb340b8f8c6c5529498dd3
Patch Set 1 #Patch Set 2 : Add unit test, check for partial lines, increment read offset #Patch Set 3 : Anonymize logs response #Patch Set 4 : Add commenting #Patch Set 5 : Remove TODO, already done #
Total comments: 21
Patch Set 6 : Respond to afakhry's comments on .cc and .h files #
Total comments: 12
Patch Set 7 : afakhry's comments for unit test #Patch Set 8 : delete mutable_file() #
Total comments: 4
Patch Set 9 : Add WeakPtrFactory; Move RunUntilIdle() into FetchFromSource() #Patch Set 10 : Rebased #Patch Set 11 : Fixed CL dependency #Patch Set 12 : Directly read from file into string #
Total comments: 4
Patch Set 13 : Resize string; Skip anonymizer if empty string #Patch Set 14 : Rebased; Updated to new TaskTraits syntax #Patch Set 15 : Creating new file must be accompanied by FLAG_WRITE #
Total comments: 2
Patch Set 16 : Add TestBrowserThreadBundle #
Total comments: 3
Messages
Total messages: 68 (29 generated)
Description was changed from ========== WIP: Add SingleLogSource BUG= ========== to ========== Add new SystemLogsSource type: SingleLogSource This patch adds a new type of SystemLogsSource, SingleLogSource, which provides a single log source in contrast to the many log sources provided by the other implementations of SystemLogsSource. The SingleLogSource currently supports incremental read of a limited set of log files. It can be expanded to include other log files and other log sources. BUG=chromium:715263 ==========
sque@chromium.org changed reviewers: + rkc@chromium.org
rkc@chromium.org changed reviewers: + afakhry@chromium.org
Seems fine to me but I'll defer to the owner of system logs, Ahmed for a full review.
https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:31: std::string GetLogFileSourceFilename(const std::string& source_name) { This can now be a simple switch statement if we use enum class. The above struct and kLogFileSources[] will not be needed. You'd probably need another member variable to store source_name_. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:41: SingleLogSource::SingleLogSource() : SystemLogsSource(""), num_bytes_read_(0) {} Please remove this default constructor. It seems useless. One can't modify the object afterwards if this constructor was called. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:71: size_t length = file_.GetLength(); Nit: const here and everywhere below please. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:84: std::string result_string; Why don't you read into the string right away and avoid the copying? https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:98: (*result)[source_name()] = anonymizer_.Anonymize(result_string); result->emplace(source_name_, anonymizer_.Anonymize(result_string)); https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/system_logs/single_log_source.h (right): https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.h:20: explicit SingleLogSource(const std::string& source_name); How would users of this class know what |source_name| to use? We need to be clear about what sources this class supports. Please add something like the following: enum class SupportedSource { // For /var/log/messages. kMessages, // For /var/log/ui/ui.LATEST. kUiLatest, }; Now have your constructor take this enum. SingleLogSource(SupportedSource source); This is much better. No string comparisons or room for mis-spelling a source name, and it's quite clear what sources can be retrieved by this class. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.h:25: SingleLogSource& operator=(const SingleLogSource&) = delete; Remove these line entirely and replace by DISALLOW_COPY_AND_ASSIGN(SingleLogSource); as the last line of this class. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.h:28: // the read is complete. Remove this comment and replace with: // system_logs::SystemLogsSource: Documentation of override methods are always added in the base classes where they were declared. We don't repeat them in sub classes. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.h:46: feedback::AnonymizerTool anonymizer_; I want to understand how this class will be used in the future. We do have the ScrubbedSystemLogsFetcher that anonymizes the logs when they're fetched. Will this class be used separately? https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right): https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:2: // Use of this source code is governed by a BSD-style license that can be I will look at this test when you handle the previous comments.
https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:31: std::string GetLogFileSourceFilename(const std::string& source_name) { On 2017/05/06 01:12:13, afakhry wrote: > This can now be a simple switch statement if we use enum class. The above struct > and kLogFileSources[] will not be needed. > > You'd probably need another member variable to store source_name_. Done. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:41: SingleLogSource::SingleLogSource() : SystemLogsSource(""), num_bytes_read_(0) {} On 2017/05/06 01:12:13, afakhry wrote: > Please remove this default constructor. It seems useless. One can't modify the > object afterwards if this constructor was called. Done. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:71: size_t length = file_.GetLength(); On 2017/05/06 01:12:13, afakhry wrote: > Nit: const here and everywhere below please. Done. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:84: std::string result_string; On 2017/05/06 01:12:13, afakhry wrote: > Why don't you read into the string right away and avoid the copying? ReadAtCurrentPos() writes to a char*, but std::string does not expose a mutable char*, only const char*. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:98: (*result)[source_name()] = anonymizer_.Anonymize(result_string); On 2017/05/06 01:12:13, afakhry wrote: > result->emplace(source_name_, anonymizer_.Anonymize(result_string)); Done. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/system_logs/single_log_source.h (right): https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.h:20: explicit SingleLogSource(const std::string& source_name); On 2017/05/06 01:12:13, afakhry wrote: > How would users of this class know what |source_name| to use? We need to be > clear about what sources this class supports. Please add something like the > following: > > enum class SupportedSource { > // For /var/log/messages. > kMessages, > > // For /var/log/ui/ui.LATEST. > kUiLatest, > }; > > Now have your constructor take this enum. > > SingleLogSource(SupportedSource source); > > This is much better. No string comparisons or room for mis-spelling a source > name, and it's quite clear what sources can be retrieved by this class. Done. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.h:25: SingleLogSource& operator=(const SingleLogSource&) = delete; On 2017/05/06 01:12:13, afakhry wrote: > Remove these line entirely and replace by > > DISALLOW_COPY_AND_ASSIGN(SingleLogSource); > > as the last line of this class. Done. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.h:28: // the read is complete. On 2017/05/06 01:12:13, afakhry wrote: > Remove this comment and replace with: > > // system_logs::SystemLogsSource: > > Documentation of override methods are always added in the base classes where > they were declared. We don't repeat them in sub classes. Done. https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.h:46: feedback::AnonymizerTool anonymizer_; On 2017/05/06 01:12:13, afakhry wrote: > I want to understand how this class will be used in the future. > > We do have the ScrubbedSystemLogsFetcher that anonymizes the logs when they're > fetched. > > Will this class be used separately? I have no plans to merge this with the existing ScrubbedSystemLogsFetcher and SystemLogsFetcherBase. I'm not sure how practical it is to refactor the existing stuff to use this class, since this class will need to be further modified to handle different sources. But merging is certainly an option, should someone else decide to pursue it.
https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:84: std::string result_string; On 2017/05/06 14:19:43, Simon Que wrote: > On 2017/05/06 01:12:13, afakhry wrote: > > Why don't you read into the string right away and avoid the copying? > > ReadAtCurrentPos() writes to a char*, but std::string does not expose a mutable > char*, only const char*. You can use &result_string[0]. C++17 should provide CharT* data(); [ http://en.cppreference.com/w/cpp/string/basic_string/data ] But until then, you can use &string[0] as long as your sting is resized properly. This way you're using the string as a wrapper for your buffer. Please avoid unnecessary copying as much as possible. Those log files are big and it doesn't make sense to copy them around. https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:23: return "/var/log/ui/ui.LATEST"; The question that I really want to ask is why do we need this functionality at all? Debugd gives us all these logs and more, and we already have them all as part of our feedbacks. So I'm just trying to understand why this is needed at all? https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right): https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:20: class TestSingleLogSource : public SingleLogSource { You don't need this class at all, or the mutable_file() accessor in SingleLogSource. Simply add a friend class SingleLogSourceTest; in SingleLogSource. This is a common pattern in our code base, that test classes are friends with the classes they test. https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:53: ~SingleLogSourceTest() override { dir_.Take(); } Why do you want to make this dir outlive SingleLogSourceTest? From Take() documentation: Caller takes ownership of the temporary directory so it won't be destroyed when this object goes out of scope. https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:107: EXPECT_EQ(1, num_callback_calls_); num_callback_calls_ should not be mutable and exposed to sub class test cases like this. Please make all members private and expose only those that you need through const accessors. Then move the functionality that requires mutating the members to the base class. https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:161: EXPECT_TRUE(WriteFile("abcdefg\n")); That's just a side effect of having the overwrite string shorter than the existing one. So, depending on the needed use cases, you might need to consider making this smarter. https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:187: EXPECT_EQ("yz\n", latest_response_); Similarly, this should be smarter at detecting overwrites depending on the expected use cases.
https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:23: return "/var/log/ui/ui.LATEST"; On 2017/05/08 19:20:48, afakhry wrote: > The question that I really want to ask is why do we need this functionality at > all? Debugd gives us all these logs and more, and we already have them all as > part of our feedbacks. So I'm just trying to understand why this is needed at > all? We are using debugd because it lets us get non-file logs, such as the output of lsusb, by running lsusb with privileged access. For reading log files, there isn't even a need for debugd. The class DebugDaemonLogSource does not give us access to a single source. It goes through a bunch of different log types and fetches them all. https://cs.chromium.org/chromium/src/chrome/browser/chromeos/system_logs/debu.... SingleLogSource gives us more flexibility in the form of: 1. Ability to fetch an entire log source. 2. Ability to read a log file incrementally. These are useful in a packaged app that is uploading these logs to a cloud logging service constantly in the background, not just when the user is requesting feedback. It's highly inefficient to send the entire contents of a log file that is growing over time. It uses a lot of memory and network bandwidth. https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right): https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:20: class TestSingleLogSource : public SingleLogSource { On 2017/05/08 19:20:48, afakhry wrote: > You don't need this class at all, or the mutable_file() accessor in > SingleLogSource. Simply add a > > friend class SingleLogSourceTest; in SingleLogSource. > > This is a common pattern in our code base, that test classes are friends with > the classes they test. Done. https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:53: ~SingleLogSourceTest() override { dir_.Take(); } On 2017/05/08 19:20:48, afakhry wrote: > Why do you want to make this dir outlive SingleLogSourceTest? > > From Take() documentation: > Caller takes ownership of the temporary directory so it won't be destroyed > when this object goes out of scope. Done. https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:107: EXPECT_EQ(1, num_callback_calls_); On 2017/05/08 19:20:48, afakhry wrote: > num_callback_calls_ should not be mutable and exposed to sub class test cases > like this. Please make all members private and expose only those that you need > through const accessors. Then move the functionality that requires mutating the > members to the base class. Done. https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:161: EXPECT_TRUE(WriteFile("abcdefg\n")); On 2017/05/08 19:20:48, afakhry wrote: > That's just a side effect of having the overwrite string shorter than the > existing one. So, depending on the needed use cases, you might need to consider > making this smarter. I'm not sure that's possible. See my analysis of tail -f, which has a similar limitation: https://docs.google.com/document/d/1FyDiP9iOs8INQH5hUjXMmBoQkrQZlRanFRDqr_22C... I'm not sure if it's worth the trouble to keep track of overwrites. The log files we are reading should not be overwritten. This test is just to ensure that the SingleLogSource class is robust enough not to break in the event of an overwrite. I'll add a comment.
https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right): https://codereview.chromium.org/2844163005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:161: EXPECT_TRUE(WriteFile("abcdefg\n")); On 2017/05/08 19:58:36, Simon Que wrote: > On 2017/05/08 19:20:48, afakhry wrote: > > That's just a side effect of having the overwrite string shorter than the > > existing one. So, depending on the needed use cases, you might need to > consider > > making this smarter. > > I'm not sure that's possible. See my analysis of tail -f, which has a similar > limitation: > > https://docs.google.com/document/d/1FyDiP9iOs8INQH5hUjXMmBoQkrQZlRanFRDqr_22C... > > I'm not sure if it's worth the trouble to keep track of overwrites. The log > files we are reading should not be overwritten. This test is just to ensure that > the SingleLogSource class is robust enough not to break in the event of an > overwrite. > > I'll add a comment. Acknowledged. https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:43: base::Bind(&SingleLogSource::ReadFile, base::Unretained(this), response), This is unsafe! Sorry for not spotting this earlier. You cannot use base::Unretained(this) here, The lifetimes of the system running PostTaskWithTraitsAndReply() might be different than the lifetime of this object. Please add a WeakPtrFactory as the last member of this class, and use it here by calling GetWeakPtr() instead of base::Unretained(this). https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right): https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:103: base::RunLoop().RunUntilIdle(); Nit: You can even move the RunUntilIdle() into FetchFromSource(). ;) This reduces repetition.
https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:43: base::Bind(&SingleLogSource::ReadFile, base::Unretained(this), response), On 2017/05/09 00:39:23, afakhry wrote: > This is unsafe! Sorry for not spotting this earlier. > > You cannot use base::Unretained(this) here, The lifetimes of the system running > PostTaskWithTraitsAndReply() might be different than the lifetime of this > object. > > Please add a WeakPtrFactory as the last member of this class, and use it here by > calling GetWeakPtr() instead of base::Unretained(this). Done. https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right): https://codereview.chromium.org/2844163005/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:103: base::RunLoop().RunUntilIdle(); On 2017/05/09 00:39:23, afakhry wrote: > Nit: You can even move the RunUntilIdle() into FetchFromSource(). ;) > > This reduces repetition. Done.
lgtm. Thanks!
The CQ bit was checked by sque@chromium.org
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
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2844163005/#ps180001 (title: "Rebased")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2844163005 Patch 160001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2844163005/#ps200001 (title: "Fixed CL dependency")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/system_logs/single_log_source.cc:84: std::string result_string; On 2017/05/08 19:20:47, afakhry wrote: > On 2017/05/06 14:19:43, Simon Que wrote: > > On 2017/05/06 01:12:13, afakhry wrote: > > > Why don't you read into the string right away and avoid the copying? > > > > ReadAtCurrentPos() writes to a char*, but std::string does not expose a > mutable > > char*, only const char*. > > You can use &result_string[0]. C++17 should provide > > CharT* data(); [ http://en.cppreference.com/w/cpp/string/basic_string/data ] > > But until then, you can use &string[0] as long as your sting is resized > properly. This way you're using the string as a wrapper for your buffer. > > Please avoid unnecessary copying as much as possible. Those log files are big > and it doesn't make sense to copy them around. I see you missed this comment. Please take a look. not lgtm until then.
On 2017/05/09 23:46:18, afakhry wrote: > https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... > File chrome/browser/chromeos/system_logs/single_log_source.cc (right): > > https://codereview.chromium.org/2844163005/diff/80001/chrome/browser/chromeos... > chrome/browser/chromeos/system_logs/single_log_source.cc:84: std::string > result_string; > On 2017/05/08 19:20:47, afakhry wrote: > > On 2017/05/06 14:19:43, Simon Que wrote: > > > On 2017/05/06 01:12:13, afakhry wrote: > > > > Why don't you read into the string right away and avoid the copying? > > > > > > ReadAtCurrentPos() writes to a char*, but std::string does not expose a > > mutable > > > char*, only const char*. > > > > You can use &result_string[0]. C++17 should provide > > > > CharT* data(); [ http://en.cppreference.com/w/cpp/string/basic_string/data ] > > > > But until then, you can use &string[0] as long as your sting is resized > > properly. This way you're using the string as a wrapper for your buffer. > > > > Please avoid unnecessary copying as much as possible. Those log files are big > > and it doesn't make sense to copy them around. > > I see you missed this comment. Please take a look. not lgtm until then. Done
lgtm % a couple of nits. https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:68: std::string result_string(size_to_read, ' '); Nit: I think it would be better if we default construct it, then call resize(size_to_read); This fills the new characters with the default constructed CharT() instead of spaces, which is equivalent to std::vector<char> read_result(size_to_read); [ http://en.cppreference.com/w/cpp/string/basic_string/resize ] Could potentially be better for compiler optimizations. https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:80: result_string.clear(); Nit: You might consider doing the following here: // Without clearing the string. result->emplace(source_name(), ""); return; There's no need to invoke the anonymizer on an empty string. ;)
https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:68: std::string result_string(size_to_read, ' '); On 2017/05/10 04:28:03, afakhry wrote: > Nit: I think it would be better if we default construct it, then call > resize(size_to_read); This fills the new characters with the default constructed > CharT() instead of spaces, which is equivalent to > > std::vector<char> read_result(size_to_read); > > [ http://en.cppreference.com/w/cpp/string/basic_string/resize ] > > Could potentially be better for compiler optimizations. Done. https://codereview.chromium.org/2844163005/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:80: result_string.clear(); On 2017/05/10 04:28:03, afakhry wrote: > Nit: You might consider doing the following here: > > // Without clearing the string. > result->emplace(source_name(), ""); > return; > > There's no need to invoke the anonymizer on an empty string. ;) Done.
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2844163005/#ps240001 (title: "Resize string; Skip anonymizer if empty string")
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
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2844163005/#ps260001 (title: "Rebased; Updated to new TaskTraits syntax")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2844163005/#ps280001 (title: "Creating new file must be accompanied by FLAG_WRITE")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/05/10 19:05:47, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) It's failing this check: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Does this mean it needs to be in a browser test instead of a unit test?
https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:37: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); It's failing because of this DCHECK. https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right): https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:97: base::test::ScopedTaskScheduler scheduler_; Can you also add a member of type content::TestBrowserThreadBundle to overcome those failures? To validate that it works. Build a debug version of your test, and run it to make sure you don't encounter those failures. Always run unit tests in a debug build as DCHECKs are enabled and you'd know if you made a mistake. gn gen out/debug --args="is_debug=true .... etc."
On 2017/05/11 00:56:54, afakhry wrote: > https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... > File chrome/browser/chromeos/system_logs/single_log_source.cc (right): > > https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... > chrome/browser/chromeos/system_logs/single_log_source.cc:37: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > It's failing because of this DCHECK. > > https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... > File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc (right): > > https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... > chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:97: > base::test::ScopedTaskScheduler scheduler_; > Can you also add a member of type content::TestBrowserThreadBundle to overcome > those failures? > > To validate that it works. Build a debug version of your test, and run it to > make sure you don't encounter those failures. > > Always run unit tests in a debug build as DCHECKs are enabled and you'd know if > you made a mistake. > > gn gen out/debug --args="is_debug=true .... etc." I added the content::TestBrowserThreadBundle as a member of the test class. When I ran the test, the ScopedTaskScheduler failed this CHECK: [111683:111683:0510/214358.725254:3136124137179:FATAL:scoped_task_scheduler.cc(324)] Check failed: !TaskScheduler::GetInstance(). I assumed that there was already a TaskScheduler created by the TestBrowserThreadBundle, so I removed the ScopedTaskScheduler. Now I don't see the callback being invoked, and there is also this CHECK failing: [111881:111881:0510/214622.277746:3136267689670:FATAL:weak_ptr.cc(20)] Check failed: sequence_checker_.CalledOnValidSequence() || HasOneRef(). WeakPtrs must be invalidated on the same sequenced thread.
On 2017/05/11 01:49:04, Simon Que wrote: > On 2017/05/11 00:56:54, afakhry wrote: > > > https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... > > File chrome/browser/chromeos/system_logs/single_log_source.cc (right): > > > > > https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... > > chrome/browser/chromeos/system_logs/single_log_source.cc:37: > > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > > It's failing because of this DCHECK. > > > > > https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... > > File chrome/browser/chromeos/system_logs/single_log_source_unittest.cc > (right): > > > > > https://codereview.chromium.org/2844163005/diff/280001/chrome/browser/chromeo... > > chrome/browser/chromeos/system_logs/single_log_source_unittest.cc:97: > > base::test::ScopedTaskScheduler scheduler_; > > Can you also add a member of type content::TestBrowserThreadBundle to overcome > > those failures? > > > > To validate that it works. Build a debug version of your test, and run it to > > make sure you don't encounter those failures. > > > > Always run unit tests in a debug build as DCHECKs are enabled and you'd know > if > > you made a mistake. > > > > gn gen out/debug --args="is_debug=true .... etc." > > I added the content::TestBrowserThreadBundle as a member of the test class. > > When I ran the test, the ScopedTaskScheduler failed this CHECK: > [111683:111683:0510/214358.725254:3136124137179:FATAL:scoped_task_scheduler.cc(324)] > Check failed: !TaskScheduler::GetInstance(). > > I assumed that there was already a TaskScheduler created by the > TestBrowserThreadBundle, so I removed the ScopedTaskScheduler. > > Now I don't see the callback being invoked, and there is also this CHECK > failing: > [111881:111881:0510/214622.277746:3136267689670:FATAL:weak_ptr.cc(20)] Check > failed: sequence_checker_.CalledOnValidSequence() || HasOneRef(). WeakPtrs must > be invalidated on the same sequenced thread. I figured it out... I had to replace the ScopedTaskScheduler with a ScopedTaskEnvironment.
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2844163005/#ps300001 (title: "Add TestBrowserThreadBundle")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:37: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); The real question is why do you need to force this to be on the UI thread?
On 2017/05/11 15:42:28, afakhry wrote: > https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo... > File chrome/browser/chromeos/system_logs/single_log_source.cc (right): > > https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo... > chrome/browser/chromeos/system_logs/single_log_source.cc:37: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > The real question is why do you need to force this to be on the UI thread? I saw the check in the Fetch() functions of the other classes in the same directory, so I thought that was the standard pattern for these system log source classes. For example, CommandLineLogSource: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/system_logs/comm...
https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:37: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/05/11 15:42:28, afakhry wrote: > The real question is why do you need to force this to be on the UI thread? Requiring the fetch call to be on the UI thread seems reasonable. For all the applications we know of, only UI should be making fetch calls. The actual ReadFile should not be, which it isn't.
The CQ bit was checked by sque@chromium.org
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sque@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 300001, "attempt_start_ts": 1494609935820740, "parent_rev": "fc093d3e84762bfd20224adf0e73b49d2ad34247", "commit_rev": "def38083bf08ec479bcb340b8f8c6c5529498dd3"}
Message was sent while issue was closed.
Description was changed from ========== Add new SystemLogsSource type: SingleLogSource This patch adds a new type of SystemLogsSource, SingleLogSource, which provides a single log source in contrast to the many log sources provided by the other implementations of SystemLogsSource. The SingleLogSource currently supports incremental read of a limited set of log files. It can be expanded to include other log files and other log sources. BUG=chromium:715263 ========== to ========== Add new SystemLogsSource type: SingleLogSource This patch adds a new type of SystemLogsSource, SingleLogSource, which provides a single log source in contrast to the many log sources provided by the other implementations of SystemLogsSource. The SingleLogSource currently supports incremental read of a limited set of log files. It can be expanded to include other log files and other log sources. BUG=chromium:715263 Review-Url: https://codereview.chromium.org/2844163005 Cr-Commit-Position: refs/heads/master@{#471353} Committed: https://chromium.googlesource.com/chromium/src/+/def38083bf08ec479bcb340b8f8c... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/def38083bf08ec479bcb340b8f8c...
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/2883553002/ by jamescook@chromium.org. The reason for reverting is: Broke chromeos builder: https://build.chromium.org/p/chromiumos.chromium/builders/x86-generic-tot-chr... chromeos-chrome-60.0.3098.0_alpha-r1: FAILED: obj/chrome/browser/chromeos/chromeos/single_log_source.o chromeos-chrome-60.0.3098.0_alpha-r1: i686-pc-linux-gnu-g++ -B/usr/x86_64-pc-linux-gnu/i686-pc-linux-gnu/binutils-bin/2.25.51-gold -MMD -MF obj/chrome/browser/chromeos/chromeos/single_log_source.o.d -DUSE_CRAS -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_ASH=1 -DUSE_AURA=1 -DUSE_NSS_CERTS=1 -DUSE_OZONE=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DOS_CHROMEOS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DUSE_EGL -DTOOLKIT_VIEWS=1 -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_SUPPORT_GPU=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DV8_USE_EXTERNAL_STARTUP_DATA -DMESA_EGL_NO_X11_HEADERS -DLEVELDB_PLATFORM_CHROMIUM=1 -DFEATURE_ENABLE_VOICEMAIL -DEXPAT_RELATIVE_PATH -DGTEST_RELATIVE_PATH -DNO_SOUND_SYSTEM -DWEBRTC_CHROMIUM_BUILD -DWEBRTC_POSIX -DWEBRTC_LINUX -DCHROMEOS -I../../../../../../../home/chrome-bot/chrome_root/src -Igen -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/khronos -I../../../../../../../home/chrome-bot/chrome_root/src/gpu -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/protobuf/src -Igen/protoc_out -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/protobuf/src -I../../../../../../../home/chrome-bot/chrome_root/src/skia/config -I../../../../../../../home/chrome-bot/chrome_root/src/skia/ext -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/c -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/config -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/core -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/effects -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/images -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/lazy -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/pathops -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/pdf -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/pipe -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/ports -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/utils -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/third_party/vulkan -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/include/gpu -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/src/gpu -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/skia/src/sksl -Igen -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/ced/src -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/icu/source/common -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/icu/source/i18n -Igen -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/libwebm/source -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/boringssl/src/include -I/build/x86-generic/usr/include/nss -I/build/x86-generic/usr/include/nspr -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/WebKit -Igen/third_party/WebKit -I../../../../../../../home/chrome-bot/chrome_root/src/v8/include -Igen/v8/include -Igen/components/metrics/proto -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/re2/src -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/mesa/src/include -Igen -I/build/x86-generic/usr/include/dbus-1.0 -I/build/x86-generic/usr/lib/dbus-1.0/include -Igen -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/cacheinvalidation/overrides -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/cacheinvalidation/src -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/leveldatabase -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/leveldatabase/src -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/leveldatabase/src/include -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/libusb/src/libusb -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/webrtc_overrides -I../../../../../../../home/chrome-bot/chrome_root/src/testing/gtest/include -I../../../../../../../home/chrome-bot/chrome_root/src/third_party -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/webrtc_overrides -I../../../../../../../home/chrome-bot/chrome_root/src/third_party -I../../../../../../../home/chrome-bot/chrome_root/src/third_party/zlib -fno-strict-aliasing -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -pipe -m32 -msse2 -mfpmath=sse -mmmx -pthread -Wall -Werror -Wno-unused-local-typedefs -Wno-maybe-uninitialized -Wno-missing-field-initializers -Wno-unused-parameter -Os -fno-ident -fdata-sections -ffunction-sections -fno-omit-frame-pointer -g2 -gsplit-dwarf --sysroot=../../../../../../../build/x86-generic -fvisibility=hidden -std=gnu++11 -Wno-narrowing -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -march=i686 -pipe -march=i686 -pipe -pipe -march=i686 -mfpmath=sse -mmmx -msse -msse2 -msse3 -D__google_stl_debug_vector=1 -femit-struct-debug-reduced -c ../../../../../../../home/chrome-bot/chrome_root/src/chrome/browser/chromeos/system_logs/single_log_source.cc -o obj/chrome/browser/chromeos/chromeos/single_log_source.o chromeos-chrome-60.0.3098.0_alpha-r1: ../../../../../../../home/chrome-bot/chrome_root/src/chrome/browser/chromeos/system_logs/single_log_source.cc: In function 'std::string system_logs::{anonymous}::GetLogFileSourceFilename(system_logs::SingleLogSource::SupportedSource)': chromeos-chrome-60.0.3098.0_alpha-r1: ../../../../../../../home/chrome-bot/chrome_root/src/chrome/browser/chromeos/system_logs/single_log_source.cc:25:1: error: control reaches end of non-void function [-Werror=return-type] .
Message was sent while issue was closed.
jamescook@chromium.org changed reviewers: + jamescook@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/system_logs/single_log_source.cc (right): https://codereview.chromium.org/2844163005/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/system_logs/single_log_source.cc:25: } I think this needs a NOTREACHED(); return; |