Dear Joe Hershberger,
In message 1348264998-27323-2-git-send-email-joe.hershberger@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);elsereturn -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';} elsereturn -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