You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Cong Wang <cw...@twopensource.com> on 2016/01/05 04:45:10 UTC

Review Request 41911: Added a test case for corrupt packets

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

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

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
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

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
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.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
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.

Posted by Cong Wang <xi...@gmail.com>.
-----------------------------------------------------------
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

Posted by Cong Wang <xi...@gmail.com>.

> On Jan. 20, 2016, 6:12 p.m., Ian Downes wrote:
> > src/tests/containerizer/port_mapping_tests.cpp, line 1027
> > <https://reviews.apache.org/r/41911/diff/3/?file=1197167#file1197167line1027>
> >
> >     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

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
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
> 
>

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

Posted by Cong Wang <xi...@gmail.com>.
-----------------------------------------------------------
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

Posted by Cong Wang <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41911/
-----------------------------------------------------------

(Updated Jan. 5, 2016, 6:03 p.m.)


Review request for mesos, Ian Downes and Jie Yu.


Repository: mesos


Description (updated)
-------

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

Posted by Cong Wang <xi...@gmail.com>.
-----------------------------------------------------------
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