We pass a pointer to the flags to dabitsysctl, not an integer. Adjust the handler to accept a poitner to a da_flags.
ClosedPublic
Actions

Authored by imp on Feb 21 2020, 9:57 PM.

Details

Reviewers

cem

Commits

rS358233: We pass a pointer to the flags to dabitsysctl, not an integer. Adjust the

Summary

We pass a pointer to the flags to dabitsysctl, not an integer. Adjust the
handler to accept a poitner to a u_int. To make the type of the softc flags
stable and defined, make it a u_int. Cast the enum types to u_int for arg2 so
when passing to dabitsysctl it's a u_int.

Noticed by: emax@

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 29525
Build 27392: arc lint + arc unit

Event Timeline

imp created this revision.Feb 21 2020, 9:57 PM

Harbormaster completed remote builds in B29524: Diff 68651.Feb 21 2020, 9:57 PM

The change looks more correct than the existing logic, but I don't think an enum pointer type is right either.

sys/cam/scsi/scsi_da.c
2622–2623	`da_flags` is an enum type and probably wrong for an actual flags field that could have multiple bits set (same issue in the softc::flags field). If anything, `test` should be of type `da_flags`, and `flags` not.

Update to u_int

Harbormaster completed remote builds in B29525: Diff 68654.Feb 21 2020, 10:23 PM

imp edited the summary of this revision. (Show Details)Feb 21 2020, 10:23 PM

imp added inline comments.

sys/cam/scsi/scsi_da.c
2622–2623	da_flags is an enum type and probably wrong for an actual flags field that could have multiple bits set (same issue in the softc::flags field). enum in C++ acts that way. enum on C is just an int-ish type. this is a source of much grief in my past lives... If anything, test should be of type da_flags, and flags not. I think test should be as well... However, I'm thinking that making it all u_int might actually be better (all here is softc flags, and the two items here since that would let us hoist this function into sysctl.

LGTM.

sys/cam/scsi/scsi_da.c
2622–2623	I don't know if you're right about the language (you probably are) but compilers (largely written in C++) and static analysis tools are starting to assume things like "only defined enum values are valid values for an enum variable" so IMO it's probably easier to just follow the same model.

This revision is now accepted and ready to land.Feb 21 2020, 10:31 PM

Closed by commit rS358233: We pass a pointer to the flags to dabitsysctl, not an integer. Adjust the (authored by imp). · Explain WhyFeb 21 2020, 10:44 PM

This revision was automatically updated to reflect the committed changes.

imp added a commit: rS358233: We pass a pointer to the flags to dabitsysctl, not an integer. Adjust the.