To be more robust since the checking is performed where the interface
is referenced.
While here, remove a redundant check from if_vmove_loan().
MFC after: 2 weeks
Details
Details
- Reviewers
glebius kp pouria - Group Reviewers
network - Commits
- rG00d96da231d0: ifnet: Add some sanity checks
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I'm not sure how these assertions will be beneficial if we have to add them to every function of the ifp [un]linking.
IMHO, KASSERT/MPASS are existing to verify programmatic assumptions.
And yes, these changes are definitely verifies programmatic assumptions.
However, I don't think that using them to verify every programmatic assumptions per function is appropriate.
| sys/net/if.c | ||
|---|---|---|
| 2118–2121 | while you're here, could you please fix its style too? | |
| sys/net/if.c | ||
|---|---|---|
| 2118–2121 |
The flag IFF_DYING is a bit useless in practices. The writes to if_flags is not lock protected, hence a thread is not guareenteed to see it. Also it is a bit confusing to see a IFF_DYING interface on a "alive" list. I believe the only valid consumer of IFF_DYING is the driver, to prevent potential race condition on ioctls. Anyway this flag do not belong to if_flags. There're plans to remove IFF_DYING. See also https://reviews.freebsd.org/D49412. | |
Comment Actions
For example assertions in if_unlink_ifnet(), there're multiple places, if_detach() / if_vmove_loan() / if_vmove_reclaim() / vnet_if_return() want them. You do not want to repeat everywhere.
And yes, these changes are definitely verifies programmatic assumptions.
However, I don't think that using them to verify every programmatic assumptions per function is appropriate.
For assertions in ifunit() / ifunit_ref(), they guarantee the ifp is in good shape when being used, also they serve a document to developers. The net stack evolves, but the document lags.