Page MenuHomeFreeBSD

iflib: mark isc_driver_version as constant
ClosedPublic

Authored by jacob.e.keller_intel.com on Mar 13 2019, 10:41 PM.

Details

Summary

The iflib core never modifies the isc_driver_version string. Allow
drivers to safely assign pointers to constant buffers by marking this
parameter const.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Drivers can sort of get around this by using an explicit cast, but that triggers a -Wcast-qual warning.

erj accepted this revision.Mar 13 2019, 11:40 PM
This revision is now accepted and ready to land.Mar 13 2019, 11:40 PM

Uhh, oh I see why it's marked as non-const.... SYSCTL_ADD_STRING with a read-only flag doesn't know to take a const value....

jacob.e.keller_intel.com planned changes to this revision.Mar 13 2019, 11:44 PM

This needs some re-work before it's acceptable.

Also cast isc_driver_version in SYSCTL_ADD_STRING

jacob.e.keller_intel.com planned changes to this revision.Mar 14 2019, 12:03 AM

This still doesn't quite work 100%, because it causes a -Wcast-qual warning....

I'm not really sure how to fix this without using clang diagnostics.

Looks like the best 'fix' would be to use _Pragma to drop the warnings in SYSCTL_ADD_STRING. That will take a bit more work to get correct.

Looks like the best 'fix' would be to use _Pragma to drop the warnings in SYSCTL_ADD_STRING. That will take a bit more work to get correct.

Probably through the addition of some sort of "__force_cast()" macro which expands to a pragma to ignore the warnings, the cast, and then the pragma to pop the ignoring...

Introduce a __drop_const macro to handle the warning

This version actually compilers without the warnings, but it *is* rather ugly to have to do it this way

sys/sys/sysctl.h
349–352 ↗(On Diff #55079)

This could be reduced to drop_const(arg = (arg)), i'm not sure if that's better or not.

I tried using __builtin_constant_p(len) && len==0 as a way to decide which expression to use, but clang wasn't smart enough to constant-fold that check, so it didn't work.

jhb added a subscriber: jhb.Mar 14 2019, 11:17 PM

I think I'd rather have a SYSCTL_ADD_CONST_STRING or the like, but presumably you'd just get the warning again. The other hack is to cast through uintptr_t and it's what we normally do via the __DECONST() macro. I would just do that unconditionally perhaps:

char *__arg = __DECONST(arg, char *);
In D19577#419330, @jhb wrote:

I think I'd rather have a SYSCTL_ADD_CONST_STRING or the like, but presumably you'd just get the warning again. The other hack is to cast through uintptr_t and it's what we normally do via the __DECONST() macro. I would just do that unconditionally perhaps:

char *__arg = __DECONST(arg, char *);

I didn't know about __DECONST. I'll take a look at it, thanks!

Yea, we could add a SYSCTL_ADD_CONST_STRING as well to be more explicit.

Use __DECONST instead of a poor reimplementation

Introduce SYSCTL_CONST_STRING and SYSCTL_ADD_CONST_STRING

I like the addition of SYSCTL_ADD_CONST_STRING since it also ensures that we enforce that we don't have CTLFLAG_WR.

sys/net/iflib.c
6233–6235 ↗(On Diff #55081)

The minimal change without the SYSCTL_ADD_CONST_STRING would be to just use __DECONST() here.

jhb added a comment.Mar 15 2019, 12:02 AM

Just need to add the new macros to sysctl.9 and document them. I think I like having the explicit const string macros.

Document the new CONST_STRING sysctl macros

jhb accepted this revision as: jhb.Mar 15 2019, 7:26 PM
gallatin accepted this revision.Mar 16 2019, 10:20 PM
erj accepted this revision.Mar 19 2019, 11:24 PM
This revision is now accepted and ready to land.Mar 19 2019, 11:24 PM
This revision was automatically updated to reflect the committed changes.