Re: Review Request 41911: Added a test case for corrupt packets.

[Cong Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41911/
---

(Updated Jan. 25, 2016, 11:01 p.m.)


Review request for mesos, Ian Downes and Jie Yu.


Changes
---

Address review comments and rebase


Summary (updated)
-

Added a test case for corrupt packets.


Repository: mesos


Description (updated)
---

Added a test case for corrupt packets.


Diffs (updated)
-

  src/tests/containerizer/port_mapping_tests.cpp 
182fe9217a5da9af603d6f9c203a1689eff4ca1b 

Diff: https://reviews.apache.org/r/41911/diff/


Testing
---

make check


Thanks,

Cong Wang

Re: Review Request 41911: Added a test case for corrupt packets

[Cong Wang]

> On Jan. 20, 2016, 6:12 p.m., Ian Downes wrote:
> > src/tests/containerizer/port_mapping_tests.cpp, line 1027
> > 
> >
> > 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?

I tried, but both constructing the packet and sending the packet require almost 
same parameters, so I am not sure if splitting in this way really improves 
anything here.


- Cong


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41911/#review115242
---


On Jan. 14, 2016, 8: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, 8: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
> 
>

Re: Review Request 41911: Added a test case for corrupt packets.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41911/#review116228
---



Patch looks great!

Reviews applied: [41158, 41911]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 25, 2016, 11:01 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41911/
> ---
> 
> (Updated Jan. 25, 2016, 11:01 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test case for corrupt packets.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 182fe9217a5da9af603d6f9c203a1689eff4ca1b 
> 
> Diff: https://reviews.apache.org/r/41911/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 41911: Added a test case for corrupt packets

[Ian Downes]

---
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)


Alphabetize (or comment why this order is needed).



src/tests/containerizer/port_mapping_tests.cpp (line 994)


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 *) [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 & 0x)+(sum >> 16);

/* Invert to get the negative in ones-complement arithmetic */
return ~sum;
}
```



src/tests/containerizer/port_mapping_tests.cpp (line 995)


Is this is standard IP checksum on the IP header? Please comment as such.



src/tests/containerizer/port_mapping_tests.cpp (line 1012)


s/-/ - /



src/tests/containerizer/port_mapping_tests.cpp (line 1027)


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)


s/s/socket/



src/tests/containerizer/port_mapping_tests.cpp (line 1047)


s/iph/ipHeader/



src/tests/containerizer/port_mapping_tests.cpp (line 1048)


s/udph/updHeader/



src/tests/containerizer/port_mapping_tests.cpp (line 1050)


What's stopping this writing beyond `datagram`?



src/tests/containerizer/port_mapping_tests.cpp (line 1082)


s/psize/pseudogramSize/
size_t?



src/tests/containerizer/port_mapping_tests.cpp (line 1086)


For multiline, please split all arguments to separate lines.



src/tests/containerizer/port_mapping_tests.cpp (line 1135)


drop the "1" when there's only a single usage.



src/tests/containerizer/port_mapping_tests.cpp (line 1149)


ditto, drop the "1".



src/tests/containerizer/port_mapping_tests.cpp (line 1172)


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
> 
>

Re: Review Request 41911: Added a test case for corrupt packets

[Cong Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41911/
---

(Updated Jan. 14, 2016, 8:03 p.m.)


Review request for mesos, Ian Downes and Jie Yu.


Changes
---

rebase and cleanup


Repository: mesos


Description
---

Add a test case to ensure no corrupt packet could be delivered to application.


Diffs (updated)
-

  src/tests/containerizer/port_mapping_tests.cpp 
e3aea53468fa00374320a8b89bdbb64f38e44b01 

Diff: https://reviews.apache.org/r/41911/diff/


Testing
---

make check


Thanks,

Cong Wang

Re: Review Request 41911: Added a test case for corrupt packets

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41911/#review114583
---


Bad patch!

Reviews applied: [41158, 41911]

Failed command: ./support/apply-review.sh -n -r 41911

Error:
 2016-01-14 21:47:53 URL:https://reviews.apache.org/r/41911/diff/raw/ 
[7349/7349] -> "41911.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must end in a period.

- Mesos ReviewBot


On Jan. 14, 2016, 8: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, 8: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
> 
>

Re: Review Request 41911: Added a test case for corrupt packets

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41911/#review112805
---


Bad patch!

Reviews applied: [41158, 41911]

Failed command: ./support/apply-review.sh -n -r 41911

Error:
 2016-01-05 09:34:31 URL:https://reviews.apache.org/r/41911/diff/raw/ 
[7359/7359] -> "41911.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must end in a period.

- Mesos ReviewBot


On Jan. 5, 2016, 3:59 a.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41911/
> ---
> 
> (Updated Jan. 5, 2016, 3:59 a.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 
> 9bcf05ec071b44156b57d8515f47ee6a8bbfdfa0 
> 
> Diff: https://reviews.apache.org/r/41911/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 41911: Added a test case for corrupt packets

[Cong Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41911/
---

(Updated Jan. 5, 2016, 3:59 a.m.)


Review request for mesos, Ian Downes and Jie Yu.


Changes
---

Cleanup


Repository: mesos


Description
---

Add a test case to ensure no corrupt packet could be delivered to application


Diffs (updated)
-

  src/tests/containerizer/port_mapping_tests.cpp 
9bcf05ec071b44156b57d8515f47ee6a8bbfdfa0 

Diff: https://reviews.apache.org/r/41911/diff/


Testing
---

make check


Thanks,

Cong Wang

Review Request 41911: Added a test case for corrupt packets

[Cong Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41911/
---

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 
9bcf05ec071b44156b57d8515f47ee6a8bbfdfa0 

Diff: https://reviews.apache.org/r/41911/diff/


Testing
---

make check


Thanks,

Cong Wang