[U-Boot] [RFC PATCH 1/5] env: Add support for callbacks to environment vars

[Wolfgang Denk]

Dear Joe Hershberger,

In message <1348264998-27323-2-git-send-email-joe.hershberger at ni.com> you wrote:
> Add support for callbacks to the "hashtable" functions.
...
> +/*
> + * Iterate through the whole list calling the callback for each found element.
> + */
> +int env_attr_walk(const char *attr_list,
> +	int (*callback)(const char *name, const char *value))
> +{
> +	const char *entry, *entry_end;
> +	char *name, *value;
> +
> +	if (!attr_list)
> +		/* list not found */
> +		return 1;
> +
> +	entry = attr_list;
> +	do {
> +		char *entry_cpy = NULL;
> +
> +		entry_end = strchr(entry, ENV_ATTR_LIST_DELIM);
> +		if (entry_end == NULL) {
> +			int entry_len = strlen(entry);
> +
> +			if (entry_len) {
> +				entry_cpy = malloc(entry_len + 1);
--------------------------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +				if (entry_cpy)
> +					strcpy(entry_cpy, entry);
> +				else
> +					return -ENOMEM;
> +			}
> +		} else {
> +			int entry_len = entry_end - entry;
> +
> +			if (entry_len) {
> +				entry_cpy = malloc(entry_len + 1);
--------------------------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +				if (entry_cpy) {
> +					strncpy(entry_cpy, entry, entry_len);
> +					entry_cpy[entry_len] = '\0';
> +				} else
> +					return -ENOMEM;
> +			}
> +		}

I find it a bit strange that a function which walks an existing list
of data structures would malloc() anything?

> +		if (entry_cpy != NULL) {
> +			value = strchr(entry_cpy, ENV_ATTR_SEP);
> +			if (value != NULL) {
> +				*value++ = '\0';
> +				value = strim(value);
> +			}
> +			name = strim(entry_cpy);
> +
> +			if (strlen(name) != 0) {
> +				int retval = 0;
> +
> +				retval = callback(name, value);
> +				if (retval) {
> +					free(entry_cpy);
> +					return retval;
> +				}
> +			}
> +		}
> +
> +		free(entry_cpy);
> +		entry = entry_end + 1;
> +	} while (entry_end != NULL);
> +
> +	return 0;
> +}

Actually, this function is unreadable to me, especially because there
is zero documentation about what exactly it is supposed to do,  which
data formats are being used, etc.

> +/*
> + * Retrieve the attributes string associated with a single name in the list
> + * There is no protection on attributes being too small for the value
> + */
> +int env_attr_lookup(const char *attr_list, const char *name, char *attributes)

Atttributes in a list?

What exactly has this to do with implementing callbacks?

[Yes, I think I do know what you have in mind, but this is based on a
lot of speculation, and I may be wrong.  And people who didn't follow
the previous discussion and the old patch seris probably have no clue
at all.]

> +	entry = strstr(attr_list, name);
> +	while (entry != NULL) {
> +		if ((entry == attr_list ||
> +		    *(entry - 1) == ENV_ATTR_LIST_DELIM ||
> +		    *(entry - 1) == ' ') &&
> +		    (*(entry + strlen(name)) == ENV_ATTR_SEP ||
> +		     *(entry + strlen(name)) == ENV_ATTR_LIST_DELIM ||
> +		     *(entry + strlen(name)) == '\0' ||
> +		     *(entry + strlen(name)) == ' '))

Factor out strlen() ?

...
> diff --git a/include/env_attr.h b/include/env_attr.h
> new file mode 100644
> index 0000000..62a3667
> --- /dev/null
> +++ b/include/env_attr.h
...
> +#define ENV_ATTR_LIST_DELIM	','
> +#define ENV_ATTR_SEP		':'
> +
> +extern int env_attr_walk(const char *attr_list,
> +	int (*callback)(const char *name, const char *value));
> +extern int env_attr_lookup(const char *attr_list, const char *name,
> +	char *attributes);

This does need documentation...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You're dead, Jim.
	-- McCoy, "The Tholian Web", stardate unknown