Details

Reviewers

shurd
erj
gallatin
jhb

Group Reviewers

iflib
manpages

Commits

rS346338: MFC r345312
rS345312: iflib: mark isc_driver_version as constant

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

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 23098
Build 22155: arc lint + arc unit

Event Timeline

jacob.e.keller_intel.com created this revision.Mar 13 2019, 10:41 PM

Herald added a reviewer: shurd. · View Herald TranscriptMar 13 2019, 10:41 PM

Herald added a subscriber: ae. · View Herald Transcript

Harbormaster completed remote builds in B23073: Diff 55040.Mar 13 2019, 10:41 PM

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....

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

Also cast isc_driver_version in SYSCTL_ADD_STRING

Harbormaster completed remote builds in B23075: Diff 55042.Mar 14 2019, 12:02 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.

In D19577#419066, @jacob.e.keller_intel.com wrote:

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...

Herald added a reviewer: iflib. · View Herald TranscriptMar 14 2019, 9:02 PM

Introduce a __drop_const macro to handle the warning

Harbormaster completed remote builds in B23095: Diff 55079.Mar 14 2019, 10:40 PM

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

jacob.e.keller_intel.com added inline comments.Mar 14 2019, 10:42 PM

sys/sys/sysctl.h
349–350	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.

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

Harbormaster completed remote builds in B23096: Diff 55081.Mar 14 2019, 11:40 PM

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