[U-Boot] [PATCH 10/12] env: acl: Add environment variable access control list

[Mike Frysinger <vapier@gentoo.org>]

On Friday 17 August 2012 16:49:44 Joe Hershberger wrote:
> --- a/common/cmd_nvedit.c
> +++ b/common/cmd_nvedit.c
>
> +#if defined(CONFIG_ENV_ACL)
> +#include <env_acl.h>
> +#endif

the header should not need protection just to be included

> +#ifdef CONFIG_ENV_ACL
> +	if (env_acl_validate_env_set_params(argc, argv) < 0)
> +		return 1;
> +#endif

have the header define env_acl_validate_env_set_params() as a return 0 static 
inline func when CONFIG_ENV_ACL isn't defined and then you can drop the ifdef 
here

> --- /dev/null
> +++ b/common/env_acl.c
>
> + * (C) Copyright 2010

fwiw, it's 2012 now

> +static int _env_acl_lookup_r(const char *name, char *attributes, int
> static_acl)
> ...
> +	entry = strstr(acl, name);
> +	while (entry != NULL) {
> +		if ((entry == acl || *(entry - 1) == ENV_ACL_LIST_DELIM ||
> +		    *(entry - 1) == ' ') &&
> +		    (*(entry + strlen(name)) == ENV_ACL_ATTR_SEP ||
> +		     *(entry + strlen(name)) == ENV_ACL_LIST_DELIM ||
> +		     *(entry + strlen(name)) == '\0' ||
> +		     *(entry + strlen(name)) == ' '))
> +			break;

is that strlen optimized away ?  i suspect not.  and even if it is, the 
duplication here is kind of ugly, so it'd be better to use a local var 
anyways.
	const char *acl_val = entry + strlen(name);

> +static int env_acl_lookup_r(const char *name, char *attributes)
> +{
> +	int ret_val;
> +	/* try the env first */
> +	ret_val = _env_acl_lookup_r(name, attributes, 0);
> +	if (ret_val != 0) {
> +		/* if not found in the env, look in the static list */
> +		ret_val = _env_acl_lookup_r(name, attributes, 1);
> +	}
> +	return ret_val;
> +}
> +
> +enum env_acl_var_type env_acl_get_type(const char *name)
> +{
> +	char *type;

const

> +static void skip_num(int hex, const char *value, const char **end,
> +	int max_digits)
> +{
> +	int i;
> +
> +	if (hex && is_hex_prefix(value))
> +		value += 2;
> +
> +	for (i = max_digits; i != 0; i--) {
> +		if (hex && !isxdigit(*value))
> +			break;
> +		if (!hex && !isdigit(*value))
> +			break;
> +		value++;
> +	}
> +	if (end != NULL)
> +		*end = value;
> +}

couldn't you use strtol and abuse the endptr field ?

> --- a/tools/env/fw_env.h
> +++ b/tools/env/fw_env.h
> 
> +#define min(x, y) ({				\
> +	typeof(x) _min1 = (x);			\
> +	typeof(y) _min2 = (y);			\
> +	(void) (&_min1 == &_min2);		\
> +	_min1 < _min2 ? _min1 : _min2; })

ugh, no.  use include/compiler.h.  you might want to look@the min/max 
already defined in include/common.h rather than duplicating another one 
locally.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120822/4371457d/attachment.pgp>