diff mbox

[5/5] ipv6: Replace magic number 1500 with ETH_MTU_SIZE (i.e. 1518)

Message ID 1462218931-4573-6-git-send-email-thuth@redhat.com
State Accepted
Headers show

Commit Message

Thomas Huth May 2, 2016, 7:55 p.m. UTC
The whole ethernet frame can be up to 1518 bytes including the ethernet
header. So this value should be used instead of 1500 when the whole
ethernet packet is affected. Since we've already got a nice define
for this value, use ETH_MTU_SIZE where it is appropriate.

This patch also removes a "memset(n->eth_frame, 0, 1500)" in send_ipv6()
to get rid of the magic value 1500 there -- it can be removed since
the whole ethernet packet is filled into the buffer right after the
memset, so there are no gaps that should be cleared first.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 clients/net-snk/app/netlib/ipv6.c | 3 +--
 clients/net-snk/app/netlib/ndp.h  | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

Comments

Andrew Jones May 3, 2016, 5:41 a.m. UTC | #1
On Mon, May 02, 2016 at 09:55:31PM +0200, Thomas Huth wrote:
> The whole ethernet frame can be up to 1518 bytes including the ethernet
> header. So this value should be used instead of 1500 when the whole
> ethernet packet is affected. Since we've already got a nice define
> for this value, use ETH_MTU_SIZE where it is appropriate.
> 
> This patch also removes a "memset(n->eth_frame, 0, 1500)" in send_ipv6()
> to get rid of the magic value 1500 there -- it can be removed since
> the whole ethernet packet is filled into the buffer right after the
> memset, so there are no gaps that should be cleared first.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  clients/net-snk/app/netlib/ipv6.c | 3 +--
>  clients/net-snk/app/netlib/ndp.h  | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/clients/net-snk/app/netlib/ipv6.c b/clients/net-snk/app/netlib/ipv6.c
> index 6aa1ea3..300c913 100644
> --- a/clients/net-snk/app/netlib/ipv6.c
> +++ b/clients/net-snk/app/netlib/ipv6.c
> @@ -501,7 +501,7 @@ int send_ipv6(int fd, void* buffer, int len)
>  
>  	memcpy(&ip_dst, &ip6h->dst, 16);
>  
> -	if(len + sizeof(struct ethhdr) > 1500)
> +	if(len + sizeof(struct ethhdr) > ETH_MTU_SIZE)
>  		return -1;
>  
>  	if ( ip6_cmp (&ip6h->src, &null_ip6))
> @@ -553,7 +553,6 @@ int send_ipv6(int fd, void* buffer, int len)
>  				send_neighbour_solicitation (fd, &ip_dst);
>  
>  				// Store the packet until we know the MAC address
> -				memset(n->eth_frame, 0, 1500);
>  				fill_ethhdr (n->eth_frame,
>  					     htons(ETHERTYPE_IPv6),
>  					     get_mac_address(),
> diff --git a/clients/net-snk/app/netlib/ndp.h b/clients/net-snk/app/netlib/ndp.h
> index 74fbd8b..7274f10 100644
> --- a/clients/net-snk/app/netlib/ndp.h
> +++ b/clients/net-snk/app/netlib/ndp.h
> @@ -48,7 +48,7 @@ struct neighbor {
>  	uint8_t times_asked;
>  	/* ... */
>  	struct neighbor *next;
> -	uint8_t eth_frame[1500]; //FIXME

I did a git-blame to try and figure out what this FIXME was indicating
needed fixing, but the patch it came from was a monolithic add-
everything patch. Was there a desire to separate the frame from the
neighbor struct?

> +	uint8_t eth_frame[ETH_MTU_SIZE];
>  	uint32_t eth_len;
>  
>  #define NB_INCOMPLETE 1
> -- 
> 1.8.3.1
> 

Reviewed-by: Andrew Jones <drjones@redhat.com>
Thomas Huth May 3, 2016, 6:29 a.m. UTC | #2
On 03.05.2016 07:41, Andrew Jones wrote:
> On Mon, May 02, 2016 at 09:55:31PM +0200, Thomas Huth wrote:
>> The whole ethernet frame can be up to 1518 bytes including the ethernet
>> header. So this value should be used instead of 1500 when the whole
>> ethernet packet is affected. Since we've already got a nice define
>> for this value, use ETH_MTU_SIZE where it is appropriate.
>>
>> This patch also removes a "memset(n->eth_frame, 0, 1500)" in send_ipv6()
>> to get rid of the magic value 1500 there -- it can be removed since
>> the whole ethernet packet is filled into the buffer right after the
>> memset, so there are no gaps that should be cleared first.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  clients/net-snk/app/netlib/ipv6.c | 3 +--
>>  clients/net-snk/app/netlib/ndp.h  | 2 +-
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/clients/net-snk/app/netlib/ipv6.c b/clients/net-snk/app/netlib/ipv6.c
>> index 6aa1ea3..300c913 100644
>> --- a/clients/net-snk/app/netlib/ipv6.c
>> +++ b/clients/net-snk/app/netlib/ipv6.c
>> @@ -501,7 +501,7 @@ int send_ipv6(int fd, void* buffer, int len)
>>  
>>  	memcpy(&ip_dst, &ip6h->dst, 16);
>>  
>> -	if(len + sizeof(struct ethhdr) > 1500)
>> +	if(len + sizeof(struct ethhdr) > ETH_MTU_SIZE)
>>  		return -1;
>>  
>>  	if ( ip6_cmp (&ip6h->src, &null_ip6))
>> @@ -553,7 +553,6 @@ int send_ipv6(int fd, void* buffer, int len)
>>  				send_neighbour_solicitation (fd, &ip_dst);
>>  
>>  				// Store the packet until we know the MAC address
>> -				memset(n->eth_frame, 0, 1500);
>>  				fill_ethhdr (n->eth_frame,
>>  					     htons(ETHERTYPE_IPv6),
>>  					     get_mac_address(),
>> diff --git a/clients/net-snk/app/netlib/ndp.h b/clients/net-snk/app/netlib/ndp.h
>> index 74fbd8b..7274f10 100644
>> --- a/clients/net-snk/app/netlib/ndp.h
>> +++ b/clients/net-snk/app/netlib/ndp.h
>> @@ -48,7 +48,7 @@ struct neighbor {
>>  	uint8_t times_asked;
>>  	/* ... */
>>  	struct neighbor *next;
>> -	uint8_t eth_frame[1500]; //FIXME
> 
> I did a git-blame to try and figure out what this FIXME was indicating
> needed fixing, but the patch it came from was a monolithic add-
> everything patch. Was there a desire to separate the frame from the
> neighbor struct?

Maybe ... or maybe the author just wondered about the exact maximum
frame size that should be used here. Without further explanation, it is
quite hard to tell, so I thought it would be better to remove it.

>> +	uint8_t eth_frame[ETH_MTU_SIZE];
>>  	uint32_t eth_len;
>>  
>>  #define NB_INCOMPLETE 1
>> -- 
>> 1.8.3.1
>>
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks!

  Thomas
diff mbox

Patch

diff --git a/clients/net-snk/app/netlib/ipv6.c b/clients/net-snk/app/netlib/ipv6.c
index 6aa1ea3..300c913 100644
--- a/clients/net-snk/app/netlib/ipv6.c
+++ b/clients/net-snk/app/netlib/ipv6.c
@@ -501,7 +501,7 @@  int send_ipv6(int fd, void* buffer, int len)
 
 	memcpy(&ip_dst, &ip6h->dst, 16);
 
-	if(len + sizeof(struct ethhdr) > 1500)
+	if(len + sizeof(struct ethhdr) > ETH_MTU_SIZE)
 		return -1;
 
 	if ( ip6_cmp (&ip6h->src, &null_ip6))
@@ -553,7 +553,6 @@  int send_ipv6(int fd, void* buffer, int len)
 				send_neighbour_solicitation (fd, &ip_dst);
 
 				// Store the packet until we know the MAC address
-				memset(n->eth_frame, 0, 1500);
 				fill_ethhdr (n->eth_frame,
 					     htons(ETHERTYPE_IPv6),
 					     get_mac_address(),
diff --git a/clients/net-snk/app/netlib/ndp.h b/clients/net-snk/app/netlib/ndp.h
index 74fbd8b..7274f10 100644
--- a/clients/net-snk/app/netlib/ndp.h
+++ b/clients/net-snk/app/netlib/ndp.h
@@ -48,7 +48,7 @@  struct neighbor {
 	uint8_t times_asked;
 	/* ... */
 	struct neighbor *next;
-	uint8_t eth_frame[1500]; //FIXME
+	uint8_t eth_frame[ETH_MTU_SIZE];
 	uint32_t eth_len;
 
 #define NB_INCOMPLETE 1