mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ian Downes" <ian.dow...@gmail.com>
Subject Re: Review Request 41911: Added a test case for corrupt packets
Date Wed, 20 Jan 2016 18:12:22 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message