From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gerlando Falauto Date: Fri, 30 Mar 2012 15:00:22 +0200 Subject: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes In-Reply-To: <201203292219.02029.marex@denx.de> References: <1321634955-5561-1-git-send-email-gerlando.falauto@keymile.com> <1323264605-13541-2-git-send-email-gerlando.falauto@keymile.com> <201203292219.02029.marex@denx.de> Message-ID: <4F75AE66.1090205@keymile.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. >> + 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? [...] >> >> /* 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(). Sorry, I'm very, very bad at picking function names. Any suggestion? >> +{ >> + 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). >> >> + /* 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