Page MenuHomeFreeBSD

if_clone: Allow maxunit to be zero
ClosedPublic

Authored by zlei on Jun 27 2024, 3:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 22, 6:46 PM
Unknown Object (File)
Tue, Oct 22, 6:46 PM
Unknown Object (File)
Tue, Oct 22, 6:46 PM
Unknown Object (File)
Mon, Oct 21, 12:57 AM
Unknown Object (File)
Mon, Oct 21, 12:57 AM
Unknown Object (File)
Mon, Oct 21, 12:57 AM
Unknown Object (File)
Mon, Oct 21, 12:57 AM
Unknown Object (File)
Mon, Oct 21, 12:57 AM
Subscribers

Details

Summary

Some drivers, e.g. if_enc(4), only allow one instance to be created, but
the KPI ifc_attach_cloner() treat zero as not limited, aka IF_MAXUNIT .

Introduce a new flag IFC_F_LIMITUNIT to indicate that the provided
maxunit is limited and should be respected.

Consumers should use the new flag if there is an intended limit.

MFC after: 2 weeks

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

zlei requested review of this revision.Jun 27 2024, 3:55 PM

Shouldn't that be done to if_clone_addreq_v2, too?

sys/net/if_clone.c
533–539

Do we really need the old behavior at all? There are no consumers in tree that initialize maxunit.
So could be:

maxunit = (req->flags & IFC_F_LIMITUNIT) ? req->maxunit : IF_MAXUNIT;

Added asserting on the compatibility of layout of struct if_clone_addreq and struct if_clone_addreq_v2 .

Shouldn't that be done to if_clone_addreq_v2, too?

The layout of struct if_clone_addreq_v2 is compatible with that of struct if_clone_addreq . i.e., maxunit of if_clone_addreq_v2 is the same with if_clone_addreq . I added a compile assert to reveal that.

sys/net/if_clone.c
533–539

Do we really need the old behavior at all? There are no consumers in tree that initialize maxunit.
So could be:

maxunit = (req->flags & IFC_F_LIMITUNIT) ? req->maxunit : IF_MAXUNIT;

That is exactly same with my first revision. Well later I decided to MFC this change and I worried 3rd party drivers may have already used the new KPI so I restored the old behavior.

IMO the breakage should be trivial to fix. But it seems neither RELNOTES nor UPDATING is the right place to warn developers. Any suggestions ?

zlei edited the summary of this revision. (Show Details)

Removed compatibility for old behavior.

This revision is now accepted and ready to land.Jul 1 2024, 10:48 PM

Sorry for delay!

Do not worry. I'm just a little eager to have this change in ;)

This revision was automatically updated to reflect the committed changes.