Page MenuHomeFreeBSD

ethernet: Move the assertion of ether header sizes back into ethernet.h
ClosedPublic

Authored by zlei on Jun 20 2025, 5:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 13, 5:36 AM
Unknown Object (File)
Sat, Sep 20, 5:08 PM
Unknown Object (File)
Sep 16 2025, 7:53 AM
Unknown Object (File)
Sep 6 2025, 3:14 AM
Unknown Object (File)
Aug 29 2025, 7:23 PM
Unknown Object (File)
Aug 29 2025, 8:48 AM
Unknown Object (File)
Aug 29 2025, 6:43 AM
Unknown Object (File)
Aug 24 2025, 12:17 PM

Details

Summary

There're lots of consumers, Ethernet drivers, libraries and applications.
It is more promising to assert the sizes in separated compilation units
rather than in if_ethersubr.c only.

Those assertions were in the header file but was moved to if_ethersubr.c
due to possible conflict of the implementation of CTASSERT [1]. Now that
the default C standard is now gnu17 [2] [3] which supports _Static_assert
natively, use _Static_assert instead of CTASSERT to avoid possible
conflicts.

While here, add an extra assertion for the size of struct ether_vlan_header.

[1] d54d93ac7f0f Move CTASSERT of ether header sizes out of the header file and into ...
[2] ca4eddea97c5 src: Use gnu17 as the default C standard for userland instead of gnu99
[3] 3a98d5701c7f sys: Use gnu17 as the default C standard for the kernel

MFC after: ???

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

zlei requested review of this revision.Jun 20 2025, 5:18 AM

I intended to MFC this to stable/14, but stable/14 still has gnu99 as the default. Modern compilers used by FreeBSD support _Static_assert so for in-tree consumes the MFC should be safe, then how about the out-of-tree consumes ?

I intended to MFC this to stable/14, but stable/14 still has gnu99 as the default.

But this header is also used outside of the base system, isn't it? This could break applications using ethernet.h compiled with -pedantic -std=c99 -Werror, for instance. I'm not sure if any exist, but an exp-run would be a good idea IMO.

I intended to MFC this to stable/14, but stable/14 still has gnu99 as the default.

But this header is also used outside of the base system, isn't it? This could break applications using ethernet.h compiled with -pedantic -std=c99 -Werror, for instance. I'm not sure if any exist, but an exp-run would be a good idea IMO.

That is exactly what I concerned. Or I push this in main and later do exp-run before the MFCing ? And any guides about how to do the exp-run ?

I intended to MFC this to stable/14, but stable/14 still has gnu99 as the default.

But this header is also used outside of the base system, isn't it? This could break applications using ethernet.h compiled with -pedantic -std=c99 -Werror, for instance. I'm not sure if any exist, but an exp-run would be a good idea IMO.

That is exactly what I concerned. Or I push this in main and later do exp-run before the MFCing ?

It would be better to do the exp-run first, I think. There may be enough failures that this change is not worthwhile.

And any guides about how to do the exp-run ?

https://docs.freebsd.org/en/articles/committers-guide/#ports-exp-run

I intended to MFC this to stable/14, but stable/14 still has gnu99 as the default.

But this header is also used outside of the base system, isn't it? This could break applications using ethernet.h compiled with -pedantic -std=c99 -Werror, for instance. I'm not sure if any exist, but an exp-run would be a good idea IMO.

That is exactly what I concerned. Or I push this in main and later do exp-run before the MFCing ?

It would be better to do the exp-run first, I think. There may be enough failures that this change is not worthwhile.

Filed exp-run request to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=287761 .

And any guides about how to do the exp-run ?

https://docs.freebsd.org/en/articles/committers-guide/#ports-exp-run

Thanks for the link !

The exp-run looks good. Any objections ?

Please include the link to the exp-run bugzilla into the commit message.

P.S. Why do you want to merge that to a stable branch?

This revision is now accepted and ready to land.Jul 14 2025, 6:01 PM

Please include the link to the exp-run bugzilla into the commit message.

P.S. Why do you want to merge that to a stable branch?

The structs has __packed attribute so it is unlikely to have wrong size. Well I'm a little paranoiac that may happen in some way :)

P.S. Why do you want to merge that to a stable branch?

The structs has __packed attribute so it is unlikely to have wrong size. Well I'm a little paranoiac that may happen in some way :)

It is not about observing a different size, but about the _Static_assert() not being support by some compiler with some flags. The risk is low of course. But practical benefits of having change in STABLE are also very low.

P.S. Why do you want to merge that to a stable branch?

The structs has __packed attribute so it is unlikely to have wrong size. Well I'm a little paranoiac that may happen in some way :)

It is not about observing a different size, but about the _Static_assert() not being support by some compiler with some flags. The risk is low of course. But practical benefits of having change in STABLE are also very low.

OK. Abort the MFC plan.