Source-Changes-D archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: CVS commit: src/sys



On 03/02/2021 21:45, David Young wrote:
On Wed, Feb 03, 2021 at 05:51:40AM +0000, Roy Marples wrote:
Module Name:	src
Committed By:	roy
Date:		Wed Feb  3 05:51:40 UTC 2021

Modified Files:
	src/sys/net: if_arp.h if_ether.h if_gre.h
	src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
	    ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h

Log Message:
Remove __packed from various network structures

They are already network aligned and adding the __packed attribute
just causes needless compiler warnings about accssing members of packed
objects.

This change looks a little hasty to me.

It looks to me like some of these structs were __packed so that
they could be read/written directly from/to any offset in a packet
chain using mtod(), which does not pay any mind to the alignment
of `*t`:

#define mtod(m, t)      ((t)((m)->m_data))

I see gre_h is accessed in that way, just for one example.  I don't
see any reason in principle that every gre_h accessed through mtod()
should be 16-bit aligned in its buffer, but that is the alignment
the compiler will expect if gre_h is not __packed.  If the actual
alignment ever differs from compiler's expectation, then there
could be a bus error or an unwanted byte rotation/shift.

It looks to me like there's a bit of cleanup to do elsewhere before
removing __packed from network structures.

ssh over a gre tunnel using erlite (mips64) and pinebook (aarch64) as both ends seems to work fine. I also tested an amd64 endpoint.

Not that I disagree with your assessment that the code can always be improved.

Roy


Home | Main Index | Thread Index | Old Index