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

Authored by zlei on Fri, Jun 20, 5:18 AM.

Details

Reviewers

emaste
imp
glebius

Group Reviewers

network

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 Skipped

Unit

Tests Skipped

Event Timeline

zlei created this revision.Fri, Jun 20, 5:18 AM

Herald added subscribers: glebius, melifaro, ae. · View Herald TranscriptFri, Jun 20, 5:18 AM

zlei requested review of this revision.Fri, Jun 20, 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 ?

In D50947#1162925, @zlei wrote:

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.

In D50947#1162949, @markj wrote:

In D50947#1162925, @zlei wrote:

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 ?

In D50947#1163271, @zlei wrote:

In D50947#1162949, @markj wrote:

In D50947#1162925, @zlei wrote:

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

In D50947#1163620, @markj wrote:

In D50947#1163271, @zlei wrote:

In D50947#1162949, @markj wrote:

In D50947#1162925, @zlei wrote:

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.Mon, Jul 14, 6:01 PM

In D50947#1171749, @glebius wrote:

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 :)