[ccan] [PATCH] argcheck: a module to check argument ranges

[Rusty Russell]

Peter Hutterer <peter.hutterer at who-t.net> writes:
> This code provides some macros to check arguments for valid value ranges.
> Consider this a mild version of assert(3), because all it does is to log
> a message and continue.
>
> These macros don't replace error handling, but they are useful in
> situations where an error is unexpected but not common, i.e.
> this shouldn't happen but if it does let me know".
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> I found these quite useful, especially when prototyping something where it's
> easier to throw in a few debug messages than provide full-scale error
> handling code. Plus, there's plenty of code where error handling isn't
> common but you'd still want to know if something is outside the expected
> range (see the state machine example in _info).

Hi Peter,

        I like this!

        FYI, what I did with the list_check() and related functions was
to hand a string.  If that was no NULL, it used that message and
aborted, eg:

        struct list_head *list_check(const struct list_head *h,
                                     const char *abortstr)
        ...
	if (abortstr) {
		fprintf(stderr,
			"%s: prev corrupt in node %p (%u) of %p\n",
			abortstr, node, count, head);
		abort();
	}
	return NULL;

This worked well because abort was normally the right thing to do, and
it was incredibly convenient to use the return value inline like so:

        list_add(list_check(h, "my list head"), &new);

That's not really possible here, since unlike pointers, ints have no
known-invalid value to pass through.  But it might be a nice trick for
pointers.

Minor comments below, feel free to ignore.

> + * By default, argcheck prints to fprintf(stderr). That can be changed by
> + * #defining ARG_CHECK_LOG_FUNC and #ARG_CHECK_LOG_FUNC_HEADER,
> + * respectively. The latter is the default template with the file/line/func
> + * information. The former is the user-supplied message (if any). All helper
> + * macros have such error messages.

You should probably use argcheck_ and ARGCHECK_ prefixes, to match your
module name (and I think it reads more clearly).

> +#ifndef CCAN_ARGCHECK_H
> +#define CCAN_ARGCHECK_H
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include <ccan/likely/likely.h>
> +
> +#ifdef ARG_CHECK_DISABLED
> +#define arg_check(condition) (condition)
> +#define arg_check_msg(condition, ...) (condition)
> +#else

This is a bit weird... it doesn't really disable the check, just the
logging, which you can do by overriding the log func.

The common assert-disabling macro is NDEBUG; consider using that to
really disable this:

        /* Still evaluate once, just always succeed. */
        #define arg_check(condition) ((condition), 1)
        #define arg_check_msg(condition, ...) ((condition, __VA_ARGS__), 1)

> +#ifndef ARG_CHECK_LOG_FUNC
> +/**
> + * ARG_CHECK_LOG_FUNC - function to print error messages (default fprintf)
> + */
> +#define ARG_CHECK_LOG_FUNC(...) fprintf(stderr, __VA_ARGS__)
> +#endif

I would define this to a real "argcheck_log" function.  That way you can
mark it COLD (and use ccan/compiler instead of ccan/likely).

> +/**
> + * arg_check_int_eq - check the argument is equal to the value.
> + */
> +#define arg_check_int_eq(arg, val) \
> +	arg_check_msg(arg == val, "argcheck invalid value: %s (%d) must be %s (%d)\n", #arg, arg, #val, val)

Consider making these real functions, to avoid the really nasty
double-eval?  It means that you'll need to make archcheck_log a hook, so
caller can override it, though.

> +/**
> + * arg_check_ptr_not_null - check that a pointer is not NULL
> + */
> +#define arg_check_ptr_not_null(arg) \
> +	arg_check_msg(arg != NULL, "argcheck: %s must not be NULL\n", #arg)
> +/**
> + * arg_check_ptr_null - check that a pointer is NULL
> + */
> +#define arg_check_ptr_null(arg) \
> +	arg_check_msg(arg == NULL, "argcheck: %s must be NULL\n", #arg)

Since you can't use a non-ptr as arg without getting a warning (at least
from gcc, and from any compiler if you turn this into a function)
consider dropping "ptr" from the name?

> +/**
> + * arg_check_str_zero - check that a string is not NULL and of zero length
> + */
> +#define arg_check_str_zero(arg) \
> +	(arg_check_str_not_null(arg) && \
> +	 arg_check_msg(arg[0] == '\0', "argcheck: %s must be of zero length\n", #arg))

s/zero/empty/ sounds better to me...

> +/**
> + * arg_check_str_null_or_zero - check that a string is either NULL or of zero length
> + */
> +#define arg_check_str_null_or_zero(arg) \
> +	(arg_check_str_null(arg) || arg_check_str_zero(arg))
> +/**
> + * arg_check_str_not_zero - check that a string is not NULL and has a length greater than 0
> + */
> +#define arg_check_str_not_zero(arg) \
> +	(arg_check_str_not_null(arg) && \
> +	 arg_check_msg(arg[0] != '\0', "argcheck: %s must not be of zero length\n", #arg))
> +/**
> + * arg_check_str_null_or_not_zero - check that a string is either NULL or has a length greater than 0
> + */
> +#define arg_check_str_null_or_not_zero(arg) \
> +	(arg_check_str_null(arg) || arg_check_str_not_zero(arg))

These too...

Cheers,
Rusty.