Re: [dpdk-dev] [PATCH] ether: mark ethernet addresses as being 2-byte aligned

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Jul 01, 2019 at 04:14:30PM +0200, Olivier Matz wrote:
> Hi Bruce,
> 
> On Mon, Jul 01, 2019 at 02:38:43PM +0100, Bruce Richardson wrote:
> > On Mon, Jul 01, 2019 at 03:11:12PM +0200, Olivier Matz wrote:
> > > Hi Bruce,
> > > 
> > > On Thu, May 16, 2019 at 04:54:57PM +0100, Bruce Richardson wrote:
> > > > When including the rte_ether.h header in applications with warnings
> > > > enabled, a warning was given because of the assumption of 2-byte
> > > > alignment of ethernet addresses when processing them.
> > > > 
> > > > .../include/rte_ether.h:149:2: warning: converting a packed ‘const
> > > > struct ether_addr’ pointer (alignment 1) to a ‘unaligned_uint16_t’
> > > > {aka ‘const short unsigned int’} pointer (alignment 2) may result
> > > > in an unaligned pointer value [-Waddress-of-packed-member] 149 |
> > > > const unaligned_uint16_t *ea_words = (const unaligned_uint16_t
> > > > *)ea; |  ^~~~~
> > > > 
> > > > Since ethernet addresses should always be aligned on a two-byte
> > > > boundary,
> > > 
> > > I'm a bit reserved about this last assumption. The ethernet address
> > > structure may be used in a private structure, whose alignment is 1.
> > > Are we sure that there is no (funny) protocol that carries unaligned
> > > ethernet addresses?
> > > 
> > > Shouldn't we change the definition of unaligned_uint16_t instead?  Or
> > > change the rte_is_broadcast_ether_addr() function?
> > > 
> > 
> > We could, but I believe the correct behaviour is to make the addresses
> > always 2-byte aligned, unless someone actually has a real-world case
> > where there is a protocol that doesn't have the data 2-byte aligned.
> 
> Maybe you missed that part of my previous answer, I'm copy it again here:
> 
>   > Although this is an ABI break, the network structures are all being
>   > renamed in this release, and a deprecation notice was previously
>   > posted for it.
> 
>   Yes, but the network renaming is identified in the release note as an
>   API break, not an ABI break.
> 
> I thought we agreed to limit ABI breakages to cases where there is no
> other solution. Here, this is surely a "small" ABI breakage, but I
> suppose there is a way to do differently.
> 
> If we really want to do that way, it's better to announce it as an ABI
> break.
>
At this stage, I'm ok with pushing this to 19.11 to follow the official
deprecation process.