Add packet logger and server

Add packet logger and server

BUG=webrtc:7426
Review-Url: https://codereview.webrtc.org/2790513002
Cr-Commit-Position: refs/heads/master@{#17788}
Committed: https://chromium.googlesource.com/external/webrtc/+/fcea39d7ce3f1601d07fb34f04635be7f98eae25

[michaelt, 2017-03-30 08:18:52]

Description was changed from

==========
Add packet logger and server

BUG=webrtc:7426
==========

to

==========
Add packet logger and server

BUG=webrtc:7426
==========

[michaelt, 2017-03-30 08:18:52]

michaelt@webrtc.org changed reviewers:
+ minyue@webrtc.org, nisse@webrtc.org, stefan@webrtc.org

[stefan-webrtc, 2017-03-30 09:22:25]

https://codereview.webrtc.org/2790513002/diff/1/webrtc/tools/network_tester/t...
File webrtc/tools/network_tester/test_controller.h (right):

https://codereview.webrtc.org/2790513002/diff/1/webrtc/tools/network_tester/t...
webrtc/tools/network_tester/test_controller.h:71:
RTC_DISALLOW_COPY_AND_ASSIGN(TestController);
New line above

[michaelt, 2017-04-06 07:35:07]

https://codereview.webrtc.org/2790513002/diff/1/webrtc/tools/network_tester/t...
File webrtc/tools/network_tester/test_controller.h (right):

https://codereview.webrtc.org/2790513002/diff/1/webrtc/tools/network_tester/t...
webrtc/tools/network_tester/test_controller.h:71:
RTC_DISALLOW_COPY_AND_ASSIGN(TestController);
On 2017/03/30 09:22:25, stefan-webrtc wrote:
> New line above

Done.

[stefan-webrtc, 2017-04-06 12:19:00]

lgtm

nisse, any comments?

[nisse-webrtc, 2017-04-06 12:46:30]

https://codereview.webrtc.org/2790513002/diff/20001/webrtc/tools/network_test...
File webrtc/tools/network_tester/BUILD.gn (right):

https://codereview.webrtc.org/2790513002/diff/20001/webrtc/tools/network_test...
webrtc/tools/network_tester/BUILD.gn:91: # Suppress warnings from the Chromium
Clang plugin (bugs.webrtc.org/163).
This shouldn't be needed for new code, right?

https://codereview.webrtc.org/2790513002/diff/20001/webrtc/tools/network_test...
File webrtc/tools/network_tester/packet_logger.cc (right):

https://codereview.webrtc.org/2790513002/diff/20001/webrtc/tools/network_test...
webrtc/tools/network_tester/packet_logger.cc:29:
packet_logger_stream_.write(&size, sizeof(size));
This looks confusing.

If the intention is to write a record with a length field followed by the data,
then I'd use log_data.length() rather than packet.ByteSize to get the
appropriate length.

If there's only a single byte length field in the file, add to CHECK or DCHECK
that length isn't too large.

If it's binary data rather than ascii, I'd also prefer uint8_t and
std::basic_string<uint8_t> over char and std::string.

[michaelt, 2017-04-06 14:42:11]

https://codereview.webrtc.org/2790513002/diff/20001/webrtc/tools/network_test...
File webrtc/tools/network_tester/packet_logger.cc (right):

https://codereview.webrtc.org/2790513002/diff/20001/webrtc/tools/network_test...
webrtc/tools/network_tester/packet_logger.cc:29:
packet_logger_stream_.write(&size, sizeof(size));
On 2017/04/06 12:46:29, nisse-webrtc wrote:
> This looks confusing.
> 
> If the intention is to write a record with a length field followed by the
data,
> then I'd use log_data.length() rather than packet.ByteSize to get the
> appropriate length.

  Done

> If there's only a single byte length field in the file, add to CHECK or DCHECK
> that length isn't too large.

  Done

> If it's binary data rather than ascii, I'd also prefer uint8_t and
> std::basic_string<uint8_t> over char and std::string. 

Protobuffer don't accept std::basic_string<uint8_t> for serialization.

I tried to make this a bit less confusing by changing the names of the variables
a bit. :)

[nisse-webrtc, 2017-04-06 15:00:28]

https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
File webrtc/tools/network_tester/packet_logger.cc (right):

https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
webrtc/tools/network_tester/packet_logger.cc:30: char proto_size =
proto_data.length();
This looks better. It would be nice with a brief comment explaining the format.

I think at least proto_size should be unsigned char or uint8_t (unless you want
to change the upper limit from 255 to 127).

[michaelt, 2017-04-10 11:48:25]

https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
File webrtc/tools/network_tester/packet_logger.cc (right):

https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
webrtc/tools/network_tester/packet_logger.cc:30: char proto_size =
proto_data.length();
Since ofstream.write just accept char* , it seams a bit strange to me to use a
uint8_t and the cast it to a char. Or would you use a different approach to
solve this problem ?

Added a comment about the format.

[nisse-webrtc, 2017-04-10 12:05:37]

https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
File webrtc/tools/network_tester/packet_logger.cc (right):

https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
webrtc/tools/network_tester/packet_logger.cc:30: char proto_size =
proto_data.length();
On 2017/04/10 11:48:25, michaelt wrote:
> Since ofstream.write just accept char* , it seams a bit strange to me to use a
> uint8_t and the cast it to a char. Or would you use a different approach to
> solve this problem ?

I guess we should follow the standard C++ conventions for writing binary
(non-text) files. Not sure what these are, though... One option might be to use
std::basic_ofstream<uint8_t>, but I honestly don't know if that's mainstream or
obscure.

> Added a comment about the format. 

Good!

[michaelt, 2017-04-10 12:57:21]

https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
File webrtc/tools/network_tester/packet_logger.cc (right):

https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
webrtc/tools/network_tester/packet_logger.cc:30: char proto_size =
proto_data.length();
with std::basic_ofstream<uint8_t> we would have problem on writing the prtobuf
string then. It seams to me miss using a char pointer, to write binary data to a
file is quite accepted in the C++ community. The concept of ofstream is even
design this way. Since we can declare we will have a binary file. But we will
still have to write the file with char pointers.

[nisse-webrtc, 2017-04-10 13:24:42]

https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
File webrtc/tools/network_tester/packet_logger.cc (right):

https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
webrtc/tools/network_tester/packet_logger.cc:30: char proto_size =
proto_data.length();
On 2017/04/10 12:57:21, michaelt wrote:
> with std::basic_ofstream<uint8_t> we would have problem on writing the prtobuf
> string then. It seams to me miss using a char pointer, to write binary data to
a
> file is quite accepted in the C++ community. The concept of ofstream is even
> design this way. Since we can declare we will have a binary file. But we will
> still have to write the file with char pointers. 

Ok, I guess we have to stick with char pointers then.

But I think

  char proto_size = proto_data.length();

will involve undefined behavior for the range 128 <= length <= 255.
To fix that, I think you either have to limit the maximum message size to 127
bytes, or use an unsigned char / uint8_t. (Not entirely sure, though, I get no
complaints from compiling with --fsanitize=undefined).

[michaelt, 2017-04-10 14:16:03]

On 2017/04/10 13:24:42, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
> File webrtc/tools/network_tester/packet_logger.cc (right):
> 
>
https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
> webrtc/tools/network_tester/packet_logger.cc:30: char proto_size =
> proto_data.length();
> On 2017/04/10 12:57:21, michaelt wrote:
> > with std::basic_ofstream<uint8_t> we would have problem on writing the
prtobuf
> > string then. It seams to me miss using a char pointer, to write binary data
to
> a
> > file is quite accepted in the C++ community. The concept of ofstream is even
> > design this way. Since we can declare we will have a binary file. But we
will
> > still have to write the file with char pointers. 
> 
> Ok, I guess we have to stick with char pointers then.
> 
> But I think
> 
>   char proto_size = proto_data.length();
> 
> will involve undefined behavior for the range 128 <= length <= 255.
> To fix that, I think you either have to limit the maximum message size to 127
> bytes, or use an unsigned char / uint8_t. (Not entirely sure, though, I get no
> complaints from compiling with --fsanitize=undefined).

I see here a implicit cast from a unsigned char to a signed char, which should
not be undefined behavior, as far that i know.

[nisse-webrtc, 2017-04-11 08:47:31]

On 2017/04/10 14:16:03, michaelt wrote:
> On 2017/04/10 13:24:42, nisse-webrtc wrote:
> >
>
https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
> > File webrtc/tools/network_tester/packet_logger.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2790513002/diff/60001/webrtc/tools/network_test...
> > webrtc/tools/network_tester/packet_logger.cc:30: char proto_size =
> > proto_data.length();
> > On 2017/04/10 12:57:21, michaelt wrote:
> > > with std::basic_ofstream<uint8_t> we would have problem on writing the
> prtobuf
> > > string then. It seams to me miss using a char pointer, to write binary
data
> to
> > a
> > > file is quite accepted in the C++ community. The concept of ofstream is
even
> > > design this way. Since we can declare we will have a binary file. But we
> will
> > > still have to write the file with char pointers. 
> > 
> > Ok, I guess we have to stick with char pointers then.
> > 
> > But I think
> > 
> >   char proto_size = proto_data.length();
> > 
> > will involve undefined behavior for the range 128 <= length <= 255.
> > To fix that, I think you either have to limit the maximum message size to
127
> > bytes, or use an unsigned char / uint8_t. (Not entirely sure, though, I get
no
> > complaints from compiling with --fsanitize=undefined).
> 
> I see here a implicit cast from a unsigned char to a signed char, which should
> not be undefined behavior, as far that i know.

Hmm. According to an answer at
http://stackoverflow.com/questions/5800946/unsigned-to-signed-conversion:

Note that converting large unsigned to signed is explicitly non-portable. The
C++11 standard says: "If the destination type is signed, the value does not
change if the source integer can be represented in the destination type.
Otherwise the result is implementation-defined." 

So it's not "undefined behavior", it's "implementation defined" behavior. And I
guess it works as expected in all C and C++ implementations we use?

lgtm, then. Even if I personally dislike this common misuse of signed char.

[minyue-webrtc, 2017-04-20 09:18:13]

lgtm

[michaelt, 2017-04-20 11:35:34]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2017-04-20 11:35:34]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
nisse@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2790513002/#ps140001 (title:
"Use of ProtoString")

[commit-bot: I haz the power, 2017-04-20 11:35:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-20 11:37:26]

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

[commit-bot: I haz the power, 2017-04-20 11:37:27]

Try jobs failed on following builders:
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19647)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24951)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23799)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/24984)

[michaelt, 2017-04-20 11:53:16]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2017-04-20 11:53:16]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2790513002/#ps160001 (title:
"Fix dep to protobuf_utils")

[commit-bot: I haz the power, 2017-04-20 11:53:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-20 12:03:37]

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

[commit-bot: I haz the power, 2017-04-20 12:03:38]

Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)

[michaelt, 2017-04-20 12:15:37]

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

[commit-bot: I haz the power, 2017-04-20 12:15:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-20 12:36:37]

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

[commit-bot: I haz the power, 2017-04-20 12:36:38]

Dry run: This issue passed the CQ dry run.

[michaelt, 2017-04-20 12:37:07]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2017-04-20 12:37:08]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2790513002/#ps180001 (title:
"Set output path")

[commit-bot: I haz the power, 2017-04-20 12:37:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-20 12:39:31]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492691827781290,
"parent_rev": "76ec9d7364ac223ac44c55c7267648245f183025", "commit_rev":
"fcea39d7ce3f1601d07fb34f04635be7f98eae25"}

[commit-bot: I haz the power, 2017-04-20 12:39:35]

Description was changed from

==========
Add packet logger and server

BUG=webrtc:7426
==========

to

==========
Add packet logger and server

BUG=webrtc:7426

Review-Url: https://codereview.webrtc.org/2790513002
Cr-Commit-Position: refs/heads/master@{#17788}
Committed:
https://chromium.googlesource.com/external/webrtc/+/fcea39d7ce3f1601d07fb34f0...
==========

[commit-bot: I haz the power, 2017-04-20 12:39:37]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/fcea39d7ce3f1601d07fb34f0...