Re: [PATCH] udp: create and send ICMPv4 to local peer when applicable

[Jon Maloy <jmaloy@redhat.com>]

On 2025-02-13 14:17, Stefano Brivio wrote:
> On Thu, 13 Feb 2025 14:07:22 -0500
> Jon Maloy <jmaloy@redhat.com> wrote:
> 
>> On 2025-02-12 16:39, Jon Maloy wrote:
>>> On 2025-02-10 19:55, David Gibson wrote:
>>>> On Sun, Feb 09, 2025 at 12:00:56PM -0500, Jon Maloy wrote:
>>>>> +    const struct flowside *toside = flowside_at_sidx(tosidx);
>>>>> +    struct in_addr oaddr = toside->oaddr.v4mapped.a4;
>>>>> +    struct in_addr eaddr = toside->eaddr.v4mapped.a4;
>>>>> +    struct iov_tail data = IOV_TAIL(iov, 1, 0);
>>>>> +    struct {
>>>>> +        struct icmphdr icmp4h;
>>>>> +        struct iphdr ip4h;
>>>>> +        struct udphdr uh;
>>>>> +        char data[8];
>>>>> +    } msg;
>>>>
>>>> Needs a ((packed)) attribute to ensure we don't get compiler padding.
>>
>> I get
>> udp.c:437:23: warning: taking address of packed member of ‘struct
>> <anonymous>’ may result in an unaligned pointer value
>> [-Waddress-of-packed-member]
>>     437 |         tap_push_ip4h(&msg.ip4h, eaddr, oaddr, udplen,
>> IPPROTO_UDP);
>>
>> Also, the sizes of these members are 8+20+8+8, so I cannot see how
>> this struct can possibly be packed or how there can be an unaligned
>> pointer.
> 
> Unaligned pointer because it's on the stack, and it can happily be at a
> semi-random point of it. Packed needs to be packed because you could
> have 8 + 20 + ("oops we are at 28 let's make it 32") 4 + 8 + 8.
> 
>> In short: ((packed)) seems unnecessary, and only causes problems.
> 
> ...well, but it still should be ((packed)) because it goes on the wire,
> and ((aligned)) because you need to access the single fields:
> 
>    __attribute__((packed, aligned(__alignof__(max_align_t))));
> 
> meaning: align it to whatever any kind of machine might possibly need.
> 
Ok. That worked. Thanks.