Dear Gerlando Falauto,
On 03/29/2012 10:19 PM, Marek Vasut wrote:
Dear Gerlando Falauto,
WD prodded me too long to review this patchset ;-D
Well, better late than never! ;-)
[...]
+#if defined(CONFIG_CMD_NET)
- else if (strcmp(name, "bootfile") == 0) {
copy_filename(BootFile, newval, sizeof(BootFile));Can you remove the camel-case here please?
That's code I just moved around... Will do, sir.
Don't call me that way, makes me feel old :D
return 0;- }
+#endif
- return 0;
+}
[...]
--- a/include/search.h +++ b/include/search.h @@ -47,6 +47,13 @@ typedef struct entry {
struct _ENTRY;
/*
- Callback function to be called for checking whether the given change
may + * be applied or not. Must return 0 for approval, 1 for denial.
- */
+typedef int (*apply_cb)(const char *name, const char *oldval,
const char *newval, int flag);Is the typedef really necessary ?
[From your other email]
I have to admit I'm not much of a fan of how you use this apply() callback, is it really necessary?
Why ask, if you already know the answer? :-)
I'm not a big fan either, seemed like the easiest approach at the time. The idea was to keep the hastable (struct hsearch_data) as decoupled as possible from the environment (env_htab which is, in fact, the only instance of struct hsearch_data).
What if the function pointer was stored within the hastable itself? Sort of a virtual method. This way we get rid of the typedef and the function pointer as a parameter altogether. The callback parameter then just becomes a boolean value (meaning, do/don't call the callback function stored within the hashtable itself). I like that much better. What do you think?
Don't we always use only one (this callback) function?
[...]
/* Flags for himport_r() */ #define H_NOCLEAR 1 /* do not clear hash table before
importing */
+#define H_FORCE 2 /* overwrite read-only/write-once
variables */
Make this 1<< x please.
OK.
#endif /* search.h */
diff --git a/lib/hashtable.c b/lib/hashtable.c index abd61c8..75b9b07 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, * himport()
*/
+/* Check whether variable name is amongst vars[] */ +static int process_var(const char *name, int nvars, char * const vars[])
You mean check_var()?
I mean is_var_in_set_or_is_set_empty().
Nice name :)
Sorry, I'm very, very bad at picking function names. Any suggestion?
The above is quite descriptive ... maybe is_var_in_set() ? And hey, don't be sorry, you're doing very good job!
+{
- int i = 0;
- /* No variables specified means process all of them */
- if (nvars == 0)
return 1;- for (i = 0; i< nvars; i++) {
if (!strcmp(name, vars[i]))return 1;- }
- debug("Skipping non-listed variable %s\n", name);
- return 0;
+}
/*
- Import linearized data into hash table.
@@ -639,7 +655,9 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, */
int himport_r(struct hsearch_data *htab,
const char *env, size_t size, const char sep, int flag)
const char *env, size_t size, const char sep, int flag,int nvars, char * const vars[],apply_cb apply){
char *data, *sp, *dp, *name, *value;
@@ -726,6 +744,8 @@ int himport_r(struct hsearch_data *htab,
*dp++ = '\0'; /* terminate name */ debug("DELETE CANDIDATE: \"%s\"\n", name);
if (!process_var(name, nvars, vars))continue; if (hdelete_r(name, htab) == 0)debug("DELETE ERROR
##############################\n");
@@ -743,10 +763,31 @@ int himport_r(struct hsearch_data *htab,
*sp++ = '\0'; /* terminate value */ ++dp;
/* Skip variables which are not supposed to be treated */if (!process_var(name, nvars, vars))continue;/* enter into hash table */ e.key = name; e.data = value;
Do you need to do this if you eventually later figure out you have no apply() callback and you did this for no reason?
You mean calling process_var()? It's two separate things.
One, filter out the variables that were not asked to be processed, if there were any (call to process_var()) Two, check whether the new value is acceptable and/or apply it (call apply()) You could have none, either, or both.
Or else, if you mean moving the e.key = name, e.data = value assignments, you're right, they should be moved down (but in that case it would be because the apply function fails, not because it's not present -- default case is always successful).
Yep, that's what I meant. OK
/* if there is an apply function, check what it has to say */if (apply != NULL) {debug("searching before calling cb function"" for %s\n", name);/** Search for variable in existing env, so to pass* its previous value to the apply callback*/hsearch_r(e, FIND,&rv, htab);debug("previous value was %s\n", rv ? rv->data : "");if (apply(name, rv ? rv->data : NULL, value, flag)) {debug("callback function refused to set"" variable %s, skipping it!\n", name);continue;}}hsearch_r(e, ENTER,&rv, htab); if (rv == NULL) { printf("himport_r: can't insert "%s=%s" into hash
table\n",
Thank you, Gerlando