[ovs-dev] [PATCH 1/5] odp-execute: Fix unaligned eth_addr pointers.

[Joe Stringer]

On 25 May 2017 at 13:35, Ben Pfaff <blp at ovn.org> wrote:
> On Thu, May 25, 2017 at 01:05:11PM -0700, Joe Stringer wrote:
>> On 24 May 2017 at 20:02, Ben Pfaff <blp at ovn.org> wrote:
>> > On Tue, May 23, 2017 at 04:02:12PM -0700, Joe Stringer wrote:
>> >> Clang 4.0 complains:
>> >>
>> >> ../lib/odp-execute.c:61:37: error: taking address of packed member 'eth_dst' of
>> >> class or structure 'eth_header' may result in an unaligned pointer value
>> >>       [-Werror,-Waddress-of-packed-member]
>> >>             ether_addr_copy_masked(&eh->eth_src, key->eth_src, mask->eth_src);
>> >>                                     ^~~~~~~~~~~
>> >> ../lib/odp-execute.c:62:37: error: taking address of packed member 'eth_dst' of
>> >> class or structure 'eth_header' may result in an unaligned pointer value
>> >>       [-Werror,-Waddress-of-packed-member]
>> >>             ether_addr_copy_masked(&eh->eth_dst, key->eth_dst, mask->eth_dst);
>> >>
>> >> Ethernet source addresses are 48 bits offset into the Ethernet header,
>> >> so taking a pointer for this is not guaranteed to be valid on all
>> >> architectures. Fix this by referencing the memory direct from the
>> >> Ethernet header pointer.
>> >>
>> >> Signed-off-by: Joe Stringer <joe at ovn.org>
>> >
>> > I don't understand--why does Clang think that there's something packed
>> > here?  I don't see any packed annotation on struct eth_header (and I
>> > don't think it needs one).
>>
>> https://github.com/openvswitch/ovs/blob/master/lib/packets.h#L398
>>
>> I believe that this is the wire-formatted version that we use for
>> assembling PDUs for protocols such as STP, so I think it needs to be
>> properly packed.
>
> Oh, oops, somehow I was blind to the difference between eth_header and
> eth_addr.
>
> But I don't actually see a need to pack this structure.  There's nothing
> in it that would cause a compile to insert padding.  It has all 16-bit
> members (including nested members), and you never get padding (on normal
> architectures) when all the members of a struct have the same size;
> otherwise arrays wouldn't work reasonably.
>
> I'll send some patches.

Ok thanks, I'll abandon this series and look for your patches.