Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using assert() for checking return value from OS functions. #37

Closed
troglobit opened this issue Oct 19, 2015 · 0 comments
Closed

Stop using assert() for checking return value from OS functions. #37

troglobit opened this issue Oct 19, 2015 · 0 comments

Comments

@troglobit
Copy link
Collaborator

I've noticed that libConfuse relies heavily on assert(). Personally I think I'd like to see more of a conservative approach to its use. I.e., only use it for internal DBC and not for, e.g. checking return value from malloc().

Since libConfuse is a library, I believe we should only assert on things that are clear internal "contract violations", e.g. invalid input to internal functions, or internal fault conditions that simply should not happen. Running out of memory is not one such internal condition in my opinion. Mostly because asserts can be disabled, by defining NDEBUG. Using assert() for failed malloc() is, put simply, laziness ... for a regular desktop system its possibly useful, but for embedded (headless) systems such behavior is rarely desired.

However, I'm a bit hesitant to simply roll out a massive change without posting my proposal here first. So here it is:

  1. On invalid input to a public API the API should return error. E.g, NULL or non-zero with errno=EINVAL
  2. On internal problems, e.g. allocating RAM, the API should return NULL or non-zero with errno set to represent the error condition, e.g. ENOMEM
  3. Only on internal consistency and sanity checks should the library assert() and cause an exit/abort for the calling program -- and this should probably be possible to enable/disable so release binaries set NDEBUG. Maybe even use --enable-maintainer-mode, or the reverse of that perhaps: --disable-asserts to the configure script for this.

Comments?

troglobit added a commit to troglobit/libconfuse that referenced this issue Oct 26, 2015
…location.

This patch removes assert()'s used for checking memory allocation:
calloc(), malloc(), strdup().  Instead libConfuse now returns NULL to
allow the caller to handle such errors more gracefully.

Some memory allocations did even have an assert(), they simply assumed
memory had been successfully allocated.  These have also been fixed.

For internal functions which previously asserted, their return value
is now verified by the public API's using them.

Some minor cleanup is also included in this commit:
- Readability: avoid assignment inside conditionals
- Whitespace for sectionalizing code

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>
troglobit added a commit to troglobit/libconfuse that referenced this issue Oct 26, 2015
…location.

This patch removes assert()'s used for checking memory allocation:
calloc(), malloc(), strdup().  Instead libConfuse now returns NULL to
allow the caller to handle such errors more gracefully.

Some memory allocations did even have an assert(), they simply assumed
memory had been successfully allocated.  These have also been fixed.

For internal functions which previously asserted, their return value
is now verified by the public API's using them.

Some minor cleanup is also included in this commit:
- Readability: avoid assignment inside conditionals
- Whitespace for sectionalizing code

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>
troglobit added a commit to troglobit/libconfuse that referenced this issue Oct 26, 2015
After comment from @peda-r:

- Handle strdup() failure in cfg_dupopt_array()
- Handle strdup() failure in cfg_opt_setnstr()

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>
troglobit added a commit to troglobit/libconfuse that referenced this issue Oct 28, 2015
…() [SQUASHME]

The cfg_opt_getval() API now returns NULL, we must check for that now so
we do not dereference invalid pointers.

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>
troglobit added a commit to troglobit/libconfuse that referenced this issue Oct 28, 2015
…dation

This patch introduces a semantic change in the behavior of libConfuse on
invalid input to API's.  Replacing assert() for argument validation with
a simple if() statement (inverted logic compared to asserts) with errno
set to EINVAL and returning cfg_false, NULL or similar on error.

Some API's, mostly cfg_*set*(), are declared to return void, others,
cfg_*getint/float/bool*(), return the actual data value.  The former set
functions will be changed in a later patch to return a status (POSIX
OK(0) or non-zero on error), and the latter will likely return zero on
error, which means the user will have to consult errno to check for
error.

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>
troglobit added a commit to troglobit/libconfuse that referenced this issue Oct 28, 2015
This patch changes the API for libConfuse set functions to return
the status of the operation: CFG_FAIL, with errno set on error,
and CFG_SUCCESS (POSIX OK) on success.

Also, the API docs in confuse.h is updated with new @return lines.

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>
troglobit added a commit to troglobit/libconfuse that referenced this issue Jan 25, 2016
Regression introduced in a81d930, for issue libconfuse#37.

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>
troglobit added a commit to troglobit/libconfuse that referenced this issue Jan 25, 2016
Regression introduced in a81d930, for issue libconfuse#37.

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant