On 03/30/2012 03:08 PM, Marek Vasut wrote:
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
Was more of a way to remark authority than age. :-)
+ 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?
Yes, but only because env is the only hashtable present.
Is that a yes or a no, then?
[...]
/* 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!
I like is_var_in_set().
Thanks,
Gerlando
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
