| Message ID | 1333391204-16318-7-git-send-email-gerlando.falauto@keymile.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear Gerlando Falauto, > When variables explicitly specified on the command line are not present > in the imported env, delete them from the running env. > If the variable is also missing from the running env, issue a warning. > > Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com> > --- > lib/hashtable.c | 48 +++++++++++++++++++++++++++++++++++++++++------- > 1 files changed, 41 insertions(+), 7 deletions(-) > > diff --git a/lib/hashtable.c b/lib/hashtable.c > index f3f47de..b3d0b64 100644 > --- a/lib/hashtable.c > +++ b/lib/hashtable.c > @@ -607,22 +607,32 @@ ssize_t hexport_r(struct hsearch_data *htab, const > char sep, * himport() > */ > > -/* Check whether variable name is amongst vars[] */ > -static int is_var_in_set(const char *name, int nvars, char * const vars[]) > +/* > + * Check whether variable 'name' is amongst vars[], > + * and remove all instances by setting the pointer to NULL > + */ > +static int is_var_in_set(const char *name, int nvars, char * vars[]) > { > int i = 0; > + int res = 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; > + if (vars[i] == NULL) > + continue; > + /* If we found it, delete all of them */ > + if (!strcmp(name, vars[i])) { > + vars[i] = NULL; > + res = 1; > + } > } > - debug("Skipping non-listed variable %s\n", name); > + if (!res) > + debug("Skipping non-listed variable %s\n", name); > > - return 0; > + return res; > } > > /* > @@ -662,9 +672,11 @@ static int is_var_in_set(const char *name, int nvars, > char * const vars[]) > > int himport_r(struct hsearch_data *htab, > const char *env, size_t size, const char sep, int flag, > - int nvars, char * const vars[], int do_apply) > + int nvars, char * const __vars[], int do_apply) > { > char *data, *sp, *dp, *name, *value; > + char *vars[nvars]; > + int i; > > /* Test for correct arguments. */ > if (htab == NULL) { > @@ -681,6 +693,10 @@ int himport_r(struct hsearch_data *htab, > memcpy(data, env, size); > dp = data; > > + /* make a local copy of the list of variables */ > + if (nvars) > + memcpy(vars, __vars, sizeof(__vars[0]) * nvars); Stupid question -- do you need the local copy at all? Why? > + > if ((flag & H_NOCLEAR) == 0) { > /* Destroy old hash table if one exists */ > debug("Destroy Hash Table: %p table = %p\n", htab, > @@ -809,6 +825,24 @@ int himport_r(struct hsearch_data *htab, > debug("INSERT: free(data = %p)\n", data); > free(data); > > + /* process variables which were not considered */ > + for (i = 0; i < nvars; i++) { > + if (vars[i] == NULL) > + continue; > + /* > + * All variables which were not deleted from the variable list > + * were not present in the imported env > + * This could mean two things: > + * a) if the variable was present in current env, we delete it > + * b) if the variable was not present in current env, we notify > + * it might be a typo > + */ > + if (hdelete_r(vars[i], htab, do_apply) == 0) > + printf("WARNING: '%s' neither in running nor in imported env!\n", > vars[i]); + else > + printf("WARNING: '%s' not in imported env, deleting it! \n", vars[i]); > + } > + > debug("INSERT: done\n"); > return 1; /* everything OK */ > }
On 04/02/2012 09:06 PM, Marek Vasut wrote: > Dear Gerlando Falauto, > >> When variables explicitly specified on the command line are not present >> in the imported env, delete them from the running env. >> If the variable is also missing from the running env, issue a warning. >> >> Signed-off-by: Gerlando Falauto<gerlando.falauto@keymile.com> >> --- >> lib/hashtable.c | 48 +++++++++++++++++++++++++++++++++++++++++------- >> 1 files changed, 41 insertions(+), 7 deletions(-) >> >> diff --git a/lib/hashtable.c b/lib/hashtable.c >> index f3f47de..b3d0b64 100644 >> --- a/lib/hashtable.c >> +++ b/lib/hashtable.c >> @@ -607,22 +607,32 @@ ssize_t hexport_r(struct hsearch_data *htab, const >> char sep, * himport() >> */ >> >> -/* Check whether variable name is amongst vars[] */ >> -static int is_var_in_set(const char *name, int nvars, char * const vars[]) >> +/* >> + * Check whether variable 'name' is amongst vars[], >> + * and remove all instances by setting the pointer to NULL >> + */ >> +static int is_var_in_set(const char *name, int nvars, char * vars[]) >> { >> int i = 0; >> + int res = 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; >> + if (vars[i] == NULL) >> + continue; >> + /* If we found it, delete all of them */ >> + if (!strcmp(name, vars[i])) { >> + vars[i] = NULL; >> + res = 1; >> + } >> } >> - debug("Skipping non-listed variable %s\n", name); >> + if (!res) >> + debug("Skipping non-listed variable %s\n", name); >> >> - return 0; >> + return res; >> } >> >> /* >> @@ -662,9 +672,11 @@ static int is_var_in_set(const char *name, int nvars, >> char * const vars[]) >> >> int himport_r(struct hsearch_data *htab, >> const char *env, size_t size, const char sep, int flag, >> - int nvars, char * const vars[], int do_apply) >> + int nvars, char * const __vars[], int do_apply) >> { >> char *data, *sp, *dp, *name, *value; >> + char *vars[nvars]; >> + int i; >> >> /* Test for correct arguments. */ >> if (htab == NULL) { >> @@ -681,6 +693,10 @@ int himport_r(struct hsearch_data *htab, >> memcpy(data, env, size); >> dp = data; >> >> + /* make a local copy of the list of variables */ >> + if (nvars) >> + memcpy(vars, __vars, sizeof(__vars[0]) * nvars); > > Stupid question -- do you need the local copy at all? Why? Because I need some way to keep track of what variables have already been taken into account, and it seemed the easiest way to do it without touching the original array (which is BTW passed as a const, at least pointer-wise). I should have written that in the commit message but I forgot. I'm not particularly fond of it either, but I'd rather do that than overwrite the original array. Not that it's needed afterwards by the caller... Of course the same information (variables "used") could be tracked in some other way (e.g. a bitmask array). I'm not sure about the binary code size, but it would just make things much more complicated to read... and it's not like this feature (selective importing) is the core of the bootloader, I guess. We can of course argue whether going through the hassle of deleting a variable specified on the command line which is not defined in the default/imported env is really the right thing to do (in other words, whether the whole patch has the right to exist!), but that's a different story. That's why I enqueued it as a separate patch. > >> + >> if ((flag& H_NOCLEAR) == 0) { >> /* Destroy old hash table if one exists */ >> debug("Destroy Hash Table: %p table = %p\n", htab, >> @@ -809,6 +825,24 @@ int himport_r(struct hsearch_data *htab, >> debug("INSERT: free(data = %p)\n", data); >> free(data); >> >> + /* process variables which were not considered */ >> + for (i = 0; i< nvars; i++) { >> + if (vars[i] == NULL) >> + continue; >> + /* >> + * All variables which were not deleted from the variable list >> + * were not present in the imported env >> + * This could mean two things: >> + * a) if the variable was present in current env, we delete it >> + * b) if the variable was not present in current env, we notify >> + * it might be a typo >> + */ >> + if (hdelete_r(vars[i], htab, do_apply) == 0) >> + printf("WARNING: '%s' neither in running nor in imported > env!\n", >> vars[i]); + else >> + printf("WARNING: '%s' not in imported env, deleting it! > \n", vars[i]); >> + } >> + >> debug("INSERT: done\n"); >> return 1; /* everything OK */ >> }
Dear Gerlando Falauto, > On 04/02/2012 09:06 PM, Marek Vasut wrote: > > Dear Gerlando Falauto, > > > >> When variables explicitly specified on the command line are not present > >> in the imported env, delete them from the running env. > >> If the variable is also missing from the running env, issue a warning. > >> > >> Signed-off-by: Gerlando Falauto<gerlando.falauto@keymile.com> > >> --- > >> > >> lib/hashtable.c | 48 > >> +++++++++++++++++++++++++++++++++++++++++------- 1 files changed, 41 > >> insertions(+), 7 deletions(-) > >> > >> diff --git a/lib/hashtable.c b/lib/hashtable.c > >> index f3f47de..b3d0b64 100644 > >> --- a/lib/hashtable.c > >> +++ b/lib/hashtable.c > >> @@ -607,22 +607,32 @@ ssize_t hexport_r(struct hsearch_data *htab, const > >> char sep, * himport() > >> > >> */ > >> > >> -/* Check whether variable name is amongst vars[] */ > >> -static int is_var_in_set(const char *name, int nvars, char * const > >> vars[]) +/* > >> + * Check whether variable 'name' is amongst vars[], > >> + * and remove all instances by setting the pointer to NULL > >> + */ > >> +static int is_var_in_set(const char *name, int nvars, char * vars[]) > >> > >> { > >> > >> int i = 0; > >> > >> + int res = 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; > >> + if (vars[i] == NULL) > >> + continue; > >> + /* If we found it, delete all of them */ > >> + if (!strcmp(name, vars[i])) { > >> + vars[i] = NULL; > >> + res = 1; > >> + } > >> > >> } > >> > >> - debug("Skipping non-listed variable %s\n", name); > >> + if (!res) > >> + debug("Skipping non-listed variable %s\n", name); > >> > >> - return 0; > >> + return res; > >> > >> } > >> > >> /* > >> > >> @@ -662,9 +672,11 @@ static int is_var_in_set(const char *name, int > >> nvars, char * const vars[]) > >> > >> int himport_r(struct hsearch_data *htab, > >> > >> const char *env, size_t size, const char sep, int flag, > >> > >> - int nvars, char * const vars[], int do_apply) > >> + int nvars, char * const __vars[], int do_apply) > >> > >> { > >> > >> char *data, *sp, *dp, *name, *value; > >> > >> + char *vars[nvars]; > >> + int i; > >> > >> /* Test for correct arguments. */ > >> if (htab == NULL) { > >> > >> @@ -681,6 +693,10 @@ int himport_r(struct hsearch_data *htab, > >> > >> memcpy(data, env, size); > >> dp = data; > >> > >> + /* make a local copy of the list of variables */ > >> + if (nvars) > >> + memcpy(vars, __vars, sizeof(__vars[0]) * nvars); > > > > Stupid question -- do you need the local copy at all? Why? > > Because I need some way to keep track of what variables have already > been taken into account, and it seemed the easiest way to do it without > touching the original array (which is BTW passed as a const, at least > pointer-wise). > I should have written that in the commit message but I forgot. > > I'm not particularly fond of it either, but I'd rather do that than > overwrite the original array. Not that it's needed afterwards by the > caller... > Of course the same information (variables "used") could be tracked in > some other way (e.g. a bitmask array). Well won't bitfield suffice then? > I'm not sure about the binary > code size, but it would just make things much more complicated to > read... and it's not like this feature (selective importing) is the core > of the bootloader, I guess. > > We can of course argue whether going through the hassle of deleting a > variable specified on the command line which is not defined in the > default/imported env is really the right thing to do (in other words, > whether the whole patch has the right to exist!), but that's a different > story. That's why I enqueued it as a separate patch. Honestly, I'm not in the position to properly argue here because I'm still making myself familiar with the env part of uboot ;-) > > >> + > >> > >> if ((flag& H_NOCLEAR) == 0) { > >> > >> /* Destroy old hash table if one exists */ > >> debug("Destroy Hash Table: %p table = %p\n", htab, > >> > >> @@ -809,6 +825,24 @@ int himport_r(struct hsearch_data *htab, > >> > >> debug("INSERT: free(data = %p)\n", data); > >> free(data); > >> > >> + /* process variables which were not considered */ > >> + for (i = 0; i< nvars; i++) { > >> + if (vars[i] == NULL) > >> + continue; > >> + /* > >> + * All variables which were not deleted from the variable list > >> + * were not present in the imported env > >> + * This could mean two things: > >> + * a) if the variable was present in current env, we delete it > >> + * b) if the variable was not present in current env, we notify > >> + * it might be a typo > >> + */ > >> + if (hdelete_r(vars[i], htab, do_apply) == 0) > >> + printf("WARNING: '%s' neither in running nor in imported > > > > env!\n", > > > >> vars[i]); + else > >> + printf("WARNING: '%s' not in imported env, deleting it! > > > > \n", vars[i]); > > > >> + } > >> + > >> > >> debug("INSERT: done\n"); > >> return 1; /* everything OK */ > >> > >> }
diff --git a/lib/hashtable.c b/lib/hashtable.c index f3f47de..b3d0b64 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -607,22 +607,32 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, * himport() */ -/* Check whether variable name is amongst vars[] */ -static int is_var_in_set(const char *name, int nvars, char * const vars[]) +/* + * Check whether variable 'name' is amongst vars[], + * and remove all instances by setting the pointer to NULL + */ +static int is_var_in_set(const char *name, int nvars, char * vars[]) { int i = 0; + int res = 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; + if (vars[i] == NULL) + continue; + /* If we found it, delete all of them */ + if (!strcmp(name, vars[i])) { + vars[i] = NULL; + res = 1; + } } - debug("Skipping non-listed variable %s\n", name); + if (!res) + debug("Skipping non-listed variable %s\n", name); - return 0; + return res; } /* @@ -662,9 +672,11 @@ static int is_var_in_set(const char *name, int nvars, char * const vars[]) int himport_r(struct hsearch_data *htab, const char *env, size_t size, const char sep, int flag, - int nvars, char * const vars[], int do_apply) + int nvars, char * const __vars[], int do_apply) { char *data, *sp, *dp, *name, *value; + char *vars[nvars]; + int i; /* Test for correct arguments. */ if (htab == NULL) { @@ -681,6 +693,10 @@ int himport_r(struct hsearch_data *htab, memcpy(data, env, size); dp = data; + /* make a local copy of the list of variables */ + if (nvars) + memcpy(vars, __vars, sizeof(__vars[0]) * nvars); + if ((flag & H_NOCLEAR) == 0) { /* Destroy old hash table if one exists */ debug("Destroy Hash Table: %p table = %p\n", htab, @@ -809,6 +825,24 @@ int himport_r(struct hsearch_data *htab, debug("INSERT: free(data = %p)\n", data); free(data); + /* process variables which were not considered */ + for (i = 0; i < nvars; i++) { + if (vars[i] == NULL) + continue; + /* + * All variables which were not deleted from the variable list + * were not present in the imported env + * This could mean two things: + * a) if the variable was present in current env, we delete it + * b) if the variable was not present in current env, we notify + * it might be a typo + */ + if (hdelete_r(vars[i], htab, do_apply) == 0) + printf("WARNING: '%s' neither in running nor in imported env!\n", vars[i]); + else + printf("WARNING: '%s' not in imported env, deleting it!\n", vars[i]); + } + debug("INSERT: done\n"); return 1; /* everything OK */ }
When variables explicitly specified on the command line are not present in the imported env, delete them from the running env. If the variable is also missing from the running env, issue a warning. Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com> --- lib/hashtable.c | 48 +++++++++++++++++++++++++++++++++++++++++------- 1 files changed, 41 insertions(+), 7 deletions(-)