Page MenuHomeFreeBSD

Add missing va_end's after corresponding va_start's to cleanup state
AbandonedPublic

Authored by ngie on Apr 23 2016, 6:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 22 2024, 8:38 PM
Unknown Object (File)
Mar 22 2024, 8:38 PM
Unknown Object (File)
Mar 22 2024, 8:38 PM
Unknown Object (File)
Mar 22 2024, 8:38 PM
Unknown Object (File)
Mar 8 2024, 2:46 PM
Unknown Object (File)
Feb 9 2024, 1:23 AM
Unknown Object (File)
Jan 18 2024, 1:51 AM
Unknown Object (File)
Jan 4 2024, 5:44 AM

Details

Reviewers
cem
Summary

Add missing va_end's after corresponding va_start's to cleanup state

Do some minor style(9) clean up in affected functions.

MFC after: 3 days
CID: 1018500-1018503
Reported by: Coverity
Sponsored by: EMC / Isilon Storage Division

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3496
Build 3536: arc lint + arc unit

Event Timeline

ngie retitled this revision from to Add missing va_end's after corresponding va_start's to cleanup state.
ngie updated this object.
ngie edited the test plan for this revision. (Show Details)
ngie added subscribers: gibbs, markj, cem and 3 others.
lib/libcam/scsi_cmdparse.c
688

This is probably not a portable assumption.

692–693

No va_start here.

lib/libcam/scsi_cmdparse.c
688

Agreed.

ngie marked an inline comment as done.Apr 28 2016, 1:29 AM
  • Remove superfluous va_end I added in the previous diff.
  • Replace va_list with va_list* in encode functions so we can use a more portable idiom for omitting NULL than bzero'ing va_list.
  • Add an assert to ensure that either the getter/putter function or the va_list is provided.
ngie marked 2 inline comments as done.Apr 28 2016, 7:54 AM
cem added a reviewer: cem.

I'm fine with using (va_list *)NULL as a sentinel value here.

I'm less excited about adding assert(3), just because assert(3) is usually kind of useless.

lib/libcam/scsi_cmdparse.c
119–120

99% of the time assert(3) will just compile out in userspace. Does this library have any other use of assert(3) (i.e. does it expect anybody to actually compile with -DDEBUG or whatever)?

131

Wowza.

This revision is now accepted and ready to land.Apr 28 2016, 3:03 PM
ngie marked 2 inline comments as done.Apr 28 2016, 6:02 PM
ngie added inline comments.
lib/libcam/scsi_cmdparse.c
119–120

It doesn't. I'll remove the asserts since they're inconsistent, but will add this to a list of items that need to be fixed in libcam, from an API perspective.

131

Yup..

ngie edited edge metadata.
ngie marked 4 inline comments as done.

Remove asserts; they're inconsistent and will likely be compiled out, as noted by cem.

TODO: fix up APIs to note requirements and be more graceful when provided bad input

This revision now requires review to proceed.Apr 28 2016, 6:04 PM
lib/libcam/scsi_cmdparse.c
119–120

assert(3) is opt-out. You need to compile with -DNDEBUG to disable them. We only do that in bsd.{lib,prog}.mk if WITHOUT_ASSERT_DEBUG is set. So they're on by default AFAIK.

lib/libcam/scsi_cmdparse.c
119–120

For some (incorrect) reason I thought -DNDEBUG got inserted automatically at -O1 and higher. It looks like that is not the case for compilers. However, toolchains (autoconf, CMake, FreeBSD build process) may insert the flag automatically for optimized builds.

ngie abandoned this revision.EditedApr 28 2016, 7:04 PM
ngie marked 2 inline comments as done.

Committed as r298753.