----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41911/#review115242 -----------------------------------------------------------
src/tests/containerizer/port_mapping_tests.cpp (line 23) <https://reviews.apache.org/r/41911/#comment176149> Alphabetize (or comment why this order is needed). src/tests/containerizer/port_mapping_tests.cpp (line 994) <https://reviews.apache.org/r/41911/#comment176151> s/ */* / This is a strange interface... why not take a `const char* data` and a `unsigned size`? and then cast the `data` appropriately. This version from `http://locklessinc.com/articles/tcp_checksum/` is clearer: ``` unsigned short checksum1(const char *buf, unsigned size) { unsigned sum = 0; int i; /* Accumulate checksum */ for (i = 0; i < size - 1; i += 2) { unsigned short word16 = *(unsigned short *) &buf[i]; sum += word16; } /* Handle odd-sized case */ if (size & 1) { unsigned short word16 = (unsigned char) buf[i]; sum += word16; } /* Fold to get the ones-complement result */ while (sum >> 16) sum = (sum & 0xFFFF)+(sum >> 16); /* Invert to get the negative in ones-complement arithmetic */ return ~sum; } ``` src/tests/containerizer/port_mapping_tests.cpp (line 995) <https://reviews.apache.org/r/41911/#comment176154> Is this is standard IP checksum on the IP header? Please comment as such. src/tests/containerizer/port_mapping_tests.cpp (line 1012) <https://reviews.apache.org/r/41911/#comment176150> s/-/ - / src/tests/containerizer/port_mapping_tests.cpp (line 1027) <https://reviews.apache.org/r/41911/#comment176267> This function is doing a lot, both constructing the packet, opening a socket and sending the packet. What about splitting this functionality? One function to construct, another to open/send/close? src/tests/containerizer/port_mapping_tests.cpp (line 1038) <https://reviews.apache.org/r/41911/#comment176178> s/s/socket/ src/tests/containerizer/port_mapping_tests.cpp (line 1047) <https://reviews.apache.org/r/41911/#comment176182> s/iph/ipHeader/ src/tests/containerizer/port_mapping_tests.cpp (line 1048) <https://reviews.apache.org/r/41911/#comment176183> s/udph/updHeader/ src/tests/containerizer/port_mapping_tests.cpp (line 1050) <https://reviews.apache.org/r/41911/#comment176180> What's stopping this writing beyond `datagram`? src/tests/containerizer/port_mapping_tests.cpp (line 1082) <https://reviews.apache.org/r/41911/#comment176184> s/psize/pseudogramSize/ size_t? src/tests/containerizer/port_mapping_tests.cpp (line 1086) <https://reviews.apache.org/r/41911/#comment176265> For multiline, please split all arguments to separate lines. src/tests/containerizer/port_mapping_tests.cpp (line 1135) <https://reviews.apache.org/r/41911/#comment176188> drop the "1" when there's only a single usage. src/tests/containerizer/port_mapping_tests.cpp (line 1149) <https://reviews.apache.org/r/41911/#comment176189> ditto, drop the "1". src/tests/containerizer/port_mapping_tests.cpp (line 1172) <https://reviews.apache.org/r/41911/#comment176264> s/> >/>>/ No need to have space between > chevrons now. - Ian Downes On Jan. 14, 2016, 12:03 p.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41911/ > ----------------------------------------------------------- > > (Updated Jan. 14, 2016, 12:03 p.m.) > > > Review request for mesos, Ian Downes and Jie Yu. > > > Repository: mesos > > > Description > ------- > > Add a test case to ensure no corrupt packet could be delivered to application. > > > Diffs > ----- > > src/tests/containerizer/port_mapping_tests.cpp > e3aea53468fa00374320a8b89bdbb64f38e44b01 > > Diff: https://reviews.apache.org/r/41911/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Cong Wang > >
