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

[Rusty Russell]

Peter Hutterer <peter.hutterer at who-t.net> writes:
> On Wed, Nov 20, 2013 at 02:08:01PM +1030, Rusty Russell wrote:
>> > +#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.

Yep, sure.

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

Afraid so.  I know it's a bit ugly.

BTW, I've been using a post-underscore, to avoid treading on reserved
namespaces, ie. argcheck_int_eq_().

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

Cool.

>> > +/**
>> > + * 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 :)

I thought "0" when I saw it originally, eg:

        int i = atoi(arg);

        if (i == 0 && !arch_check_str_zero(arg))
                errx(1, "Weird number %s", arg);

Cheers,
Rusty.