Add rotating log file stream.

Provides log sinks for rotating logs. Intended for use on mobile devices to record call logs.

BUG=4838
Committed: https://crrev.com/934119111e177770f647bd318ef8b2b943e5fb9b
Cr-Commit-Position: refs/heads/master@{#9615}

[tkchin_webrtc, 2015-07-20 18:27:00]

tkchin@webrtc.org changed reviewers:
+ jiayl@webrtc.org

[tkchin_webrtc, 2015-07-20 18:27:38]

Patchset #1 (id:1) has been deleted

[tkchin_webrtc, 2015-07-20 18:27:44]

Patchset #1 (id:20001) has been deleted

[tkchin_webrtc, 2015-07-20 18:27:48]

Patchset #1 (id:40001) has been deleted

[tkchin_webrtc, 2015-07-20 18:27:53]

Patchset #1 (id:60001) has been deleted

[tkchin_webrtc, 2015-07-20 18:27:59]

Patchset #1 (id:80001) has been deleted

[tkchin_webrtc, 2015-07-20 18:28:04]

Patchset #1 (id:100001) has been deleted

[jiayl2, 2015-07-20 23:27:06]

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/logsinks.h
File webrtc/base/logsinks.h (right):

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/logsinks.h#n...
webrtc/base/logsinks.h:51: // Log sink that uses a MobileDeviceLogSink to write
to disk.
nit: uses a MobileDeviceLogStream ...

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc
File webrtc/base/stream.cc (right):

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:603: return SR_ERROR;
Add logging for error cases?

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:614: return SR_SUCCESS;
I think we should surface the error to the client.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:629: return SR_ERROR;
ditto logging

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:658: if (!file_stream_) {
please move the impl into a private method like CloseCurrentFile, to avoid
calling Close() from RotateFiles/Open, since we don't mean to close the stream?

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:688: return file_names_[index];
The order is different in read mode and write mode, which seems confusing. Can
we always put the oldeest one first (which I think more intuitive), or the other
way?

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:691: bool FileRotatingStream::Open(bool delete_existing) {
nit: overloading adds confusion. How about moving the deleting existing files
part to the public Open, and rename this one to OpenCurrentFile?

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:717: file_stream_.reset();
ditto logging.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:727: DCHECK_EQ(mode_, kWrite);
nit: add logging for critial operations like rotating.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:729: // Starting at |last_file_index_|, delete the log
file and rename the
the name of last_file_index_ seems confusing, since it could mean "the last file
we write to".
Maybe max_file_index_?
But overall I think puting the oldest file at index 0 will be more intuitive.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:730: // previous file to be the current file.
"the current file" may be confusing with |current_file_index_|. I think an
example will be more clear, e.g. suppose the oldest file is file_2, delete
file_2, rename file_1 -> file_2, file_0 -> file_1, open file_0 as the current
file.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:767: }
Why not

do {
  current_name = it.Name();
  if (...) 
  
} while (it.Next());

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:811: DCHECK_GE(max_total_log_size, 4u);
nit: document this in the .h file

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.h
File webrtc/base/stream.h (right):

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.h#new...
webrtc/base/stream.h:421: // FileRotatingStream writes to a file in the
directory specified in the
I think the declaration and impl are big enough to be put in its own files.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.h#new...
webrtc/base/stream.h:541: class MobileDeviceLogStream : public
FileRotatingStream {
nit: this is useful not only for mobile devices, but almost everywhere since we
always have a limit on disk size or upload size.
I know it's hard to capture what it does in the name, so maybe something vague
on purpose, e.g. PriorityFileRotatingStream (since the start and the end are
given higher priority), or CallSessionRotatingStream, ...

Same comment applies to the log sink class.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream_unitt...
File webrtc/base/stream_unittest.cc (right):

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream_unitt...
webrtc/base/stream_unittest.cc:535: class FileRotatingStreamTest : public
::testing::Test {
ditto separate file.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream_unitt...
webrtc/base/stream_unittest.cc:616: TEST_F(FileRotatingStreamTest, EmptyWrite) {
nit: please comment on what the test does and verifies. Here and other places.

[tkchin_webrtc, 2015-07-21 20:31:07]

Patchset #3 (id:160001) has been deleted

[tkchin_webrtc, 2015-07-21 20:39:13]

PTAL

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/logsinks.h
File webrtc/base/logsinks.h (right):

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/logsinks.h#n...
webrtc/base/logsinks.h:51: // Log sink that uses a MobileDeviceLogSink to write
to disk.
On 2015/07/20 23:27:05, jiayl2 wrote:
> nit: uses a MobileDeviceLogStream ...

Done.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc
File webrtc/base/stream.cc (right):

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:603: return SR_ERROR;
On 2015/07/20 23:27:06, jiayl2 wrote:
> Add logging for error cases?

I didn't add logging because I thought there could be infinite loop in write
case. Eg
OnLogMessage -> Write -> <error> -> LOG(LS_ERROR) -> OnLogMessage

Maybe we can log directly to std::err instead? Wdyt?

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:614: return SR_SUCCESS;
On 2015/07/20 23:27:05, jiayl2 wrote:
> I think we should surface the error to the client.

Done.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:629: return SR_ERROR;
On 2015/07/20 23:27:06, jiayl2 wrote:
> ditto logging

Acknowledged.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:658: if (!file_stream_) {
On 2015/07/20 23:27:05, jiayl2 wrote:
> please move the impl into a private method like CloseCurrentFile, to avoid
> calling Close() from RotateFiles/Open, since we don't mean to close the
stream?

Done.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:688: return file_names_[index];
On 2015/07/20 23:27:05, jiayl2 wrote:
> The order is different in read mode and write mode, which seems confusing. Can
> we always put the oldeest one first (which I think more intuitive), or the
other
> way?

Sure, putting them in the same order in both modes is fine (ie match index to
name). I think it should stay newest first to match logrotate command behavior.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:691: bool FileRotatingStream::Open(bool delete_existing) {
On 2015/07/20 23:27:05, jiayl2 wrote:
> nit: overloading adds confusion. How about moving the deleting existing files
> part to the public Open, and rename this one to OpenCurrentFile?

Done.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:717: file_stream_.reset();
On 2015/07/20 23:27:05, jiayl2 wrote:
> ditto logging.

Acknowledged.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:727: DCHECK_EQ(mode_, kWrite);
On 2015/07/20 23:27:05, jiayl2 wrote:
> nit: add logging for critial operations like rotating.

Acknowledged.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:729: // Starting at |last_file_index_|, delete the log
file and rename the
On 2015/07/20 23:27:05, jiayl2 wrote:
> the name of last_file_index_ seems confusing, since it could mean "the last
file
> we write to".
> Maybe max_file_index_?
> But overall I think puting the oldest file at index 0 will be more intuitive.

Changed to rotation_index

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:730: // previous file to be the current file.
On 2015/07/20 23:27:05, jiayl2 wrote:
> "the current file" may be confusing with |current_file_index_|. I think an
> example will be more clear, e.g. suppose the oldest file is file_2, delete
> file_2, rename file_1 -> file_2, file_0 -> file_1, open file_0 as the current
> file. 

Done.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:767: }
On 2015/07/20 23:27:05, jiayl2 wrote:
> Why not
> 
> do {
>   current_name = it.Name();
>   if (...) 
>   
> } while (it.Next());

thx.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:811: DCHECK_GE(max_total_log_size, 4u);
On 2015/07/20 23:27:06, jiayl2 wrote:
> nit: document this in the .h file

Done.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.h
File webrtc/base/stream.h (right):

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.h#new...
webrtc/base/stream.h:421: // FileRotatingStream writes to a file in the
directory specified in the
On 2015/07/20 23:27:06, jiayl2 wrote:
> I think the declaration and impl are big enough to be put in its own files.

Done.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.h#new...
webrtc/base/stream.h:541: class MobileDeviceLogStream : public
FileRotatingStream {
On 2015/07/20 23:27:06, jiayl2 wrote:
> nit: this is useful not only for mobile devices, but almost everywhere since
we
> always have a limit on disk size or upload size.
> I know it's hard to capture what it does in the name, so maybe something vague
> on purpose, e.g. PriorityFileRotatingStream (since the start and the end are
> given higher priority), or CallSessionRotatingStream, ...
> 
> Same comment applies to the log sink class.

Done.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream_unitt...
File webrtc/base/stream_unittest.cc (right):

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream_unitt...
webrtc/base/stream_unittest.cc:535: class FileRotatingStreamTest : public
::testing::Test {
On 2015/07/20 23:27:06, jiayl2 wrote:
> ditto separate file.

Done.

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream_unitt...
webrtc/base/stream_unittest.cc:616: TEST_F(FileRotatingStreamTest, EmptyWrite) {
On 2015/07/20 23:27:06, jiayl2 wrote:
> nit: please comment on what the test does and verifies. Here and other places.

Done.

[jiayl2, 2015-07-21 23:36:19]

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc
File webrtc/base/stream.cc (right):

https://codereview.webrtc.org/1230823009/diff/140001/webrtc/base/stream.cc#ne...
webrtc/base/stream.cc:603: return SR_ERROR;
On 2015/07/21 20:39:12, tkchin_webrtc wrote:
> On 2015/07/20 23:27:06, jiayl2 wrote:
> > Add logging for error cases?
> 
> I didn't add logging because I thought there could be infinite loop in write
> case. Eg
> OnLogMessage -> Write -> <error> -> LOG(LS_ERROR) -> OnLogMessage
> 
> Maybe we can log directly to std::err instead? Wdyt?

std::err for write seems fine. For read, we can log normally.

[tkchin_webrtc, 2015-07-22 00:03:48]

Patchset #4 (id:200001) has been deleted

[tkchin_webrtc, 2015-07-22 00:04:46]

PTAL

Added some logs to error paths.

[jiayl2, 2015-07-22 17:00:11]

lgtm

[tkchin_webrtc, 2015-07-22 18:20:55]

Patchset #5 (id:240001) has been deleted

[tkchin_webrtc, 2015-07-22 18:21:45]

The CQ bit was checked by tkchin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-22 18:21:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230823009/260001

[commit-bot: I haz the power, 2015-07-22 19:08:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-22 19:08:03]

Dry run: This issue passed the CQ dry run.

[tkchin_webrtc, 2015-07-22 19:11:17]

The CQ bit was checked by tkchin@webrtc.org

[tkchin_webrtc, 2015-07-22 19:11:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jiayl@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1230823009/#ps260001 (title:
"Fix header")

[commit-bot: I haz the power, 2015-07-22 19:11:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230823009/260001

[commit-bot: I haz the power, 2015-07-22 19:12:21]

Committed patchset #5 (id:260001)

[commit-bot: I haz the power, 2015-07-22 19:12:32]

Patchset 5 (id:??) landed as
https://crrev.com/934119111e177770f647bd318ef8b2b943e5fb9b
Cr-Commit-Position: refs/heads/master@{#9615}