Dear Joe Hershberger,
In message <1348264998-27323-2-git-send-email-joe.hershber...@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: w...@denx.de
You're dead, Jim.
-- McCoy, "The Tholian Web", stardate unknown
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
