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

[Peter Hutterer]

On Wed, Nov 20, 2013 at 02:08:01PM +1030, Rusty Russell wrote:
> 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!

hehe, good. was worried this already exists somewhere and I just wasted some
time not finding it :)

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

I'll have a think about this, thanks. got some comments below that need to
be sorted first but something should be possible.
 
> 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).

yep, amended locally
 
> > +#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)

not quite what I intended, tbh. there's a reason for the madness :)
The macros work as a condition, disabling it completely is dangerous.
Consider the case of:

static void foo(int a) {
        if (!arg_check_int_ne(a, 0))
            return;

        ...
}

If argcheck is disabled, the code still works as expected, it just won't
print a warning. I actually want to use this like the above, it is simple
error handling code that prevents larger issues and I won't need to
duplicate the conditions.

In short, I really only want to disable the logging. might rename the
#define then.

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

what I'm struggling here is to keep the same log messages without a double
evaluation. the current one is really useful for defined values, it may say
something like:
  "invalid value: num_touches (11) must be less than MAX_TOUCHPOINTS (10)"

one solution is essentially having functions and wrapper macros for each
function, i.e.:

static inline int _argcheck_int_eq(int a, int b,
                                   const char *astr, const char *bstr,
                                   const char *file, int line,
                                   const char *func)
{
        if (a == b)
                return 1;

        argcheck_log(file, line, func, "%s (%d) must be %s (%d)\n",
                     astr, a, bstr, b);
        return 0;
}

#define argcheck_int_eq(arg, val) \
         _argcheck_int_eq(arg, val, #arg, #val, __FILE__, __LINE__, __func__)


is this what you're suggesting here or am I missing something?

> > +/**
> > + * 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?

this one I disagree with, but it's personal taste. Had arg_check_null first
but all other ones are prefixed with the type so I'd rather have the "ptr"
in. Plus, it's more consistent if we find other pointer checks that are not
as obvious, something like argcheck_ptr_type() or so.

> > +/**
> > + * 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...

haven't decided on that yet :)

Cheers,
   Peter
> 
> > +/**
> > + * 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.