Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
On 03/30/2012 03:00 PM, Gerlando Falauto wrote:
On 03/29/2012 10:19 PM, Marek Vasut wrote:
[...]
+#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.
Can't do that, sorry. The global symbol "BootFile" has been defined
somewhere else and it's used all over the place.
[...]
diff --git a/lib/hashtable.c b/lib/hashtable.c
index abd61c8..75b9b07 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
[...]
@@ -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).
Hhmm... sorry, the assignments need to stay where they are.
Values in e.key and e.data are used by hsearch_r() within the if statement.
+ /* 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
Gerlando
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
Dear Gerlando Falauto,
> On 03/30/2012 03:55 PM, Marek Vasut wrote:
> > Dear Gerlando Falauto,
> >
> >> On 03/30/2012 03:08 PM, Marek Vasut wrote:
> >>> Dear Gerlando Falauto,
> >>>
> On 03/29/2012 10:19 PM, Marek Vasut wrote:
> [...]
>
> >> + 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?
> >
> > Do we expect any more hashtables in the near future ?
>
> I don't think so. Anyway I would rather avoid calling functions
> belonging to the environment domain from the hastable domain directly.
> For that matter, we have a single "struct hsearch_data" instance in the
> whole project, but we keep passing it around as an argument to the
> hashtable functions.
Ah, it just came to me.
On the other hand, I have this strange feeling lib/hashtable.c is so modified
for uboot it can't be used (in a generic way) for any other storage than env
...
or am I wrong?
WD, can you comment please?
> Best,
> Gerlando
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
On 03/30/2012 03:55 PM, Marek Vasut wrote:
Dear Gerlando Falauto,
On 03/30/2012 03:08 PM, Marek Vasut wrote:
Dear Gerlando Falauto,
On 03/29/2012 10:19 PM, Marek Vasut wrote:
[...]
+ 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?
Do we expect any more hashtables in the near future ?
I don't think so. Anyway I would rather avoid calling functions
belonging to the environment domain from the hastable domain directly.
For that matter, we have a single "struct hsearch_data" instance in the
whole project, but we keep passing it around as an argument to the
hashtable functions.
Best,
Gerlando
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
Dear Gerlando Falauto,
> 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?
Do we expect any more hashtables in the near future ?
> >> [...]
> >>
> /* 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().
So be it then ;-)
Thanks!
> Thanks,
> Gerlando
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
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_FORCE2 /* 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
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
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?
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
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_FORCE2 /* 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\
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
Dear Gerlando Falauto,
WD prodded me too long to review this patchset ;-D
> The logic of checking special parameters (e.g. baudrate, stdin, stdout,
> for a valid value and/or whether can be overwritten) and applying the
> new value to the running system is now all within a single function
> env_check_apply() which can be called whenever changes are made
> to the environment, no matter if by set, default or import.
>
> With this patch env_check_apply() is only called by "env set",
> retaining previous behavior.
>
> Also allow for selectively importing/resetting variables.
>
> So add 3 new arguments to himport_r():
> o "nvars", "vars":, number and list of variables to take into account
>(0 means ALL)
>
> o "apply" callback function to check whether a variable can be
> overwritten, and possibly immediately apply the changes;
> when NULL, no check is performed.
>
> Signed-off-by: Gerlando Falauto
[...]
> + }
> +#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?
> + return 0;
> + }
> +#endif
> + return 0;
> +}
> +
> +/*
> + * Set a new environment variable,
> + * or replace or delete an existing one.
> +*/
> +int _do_env_set(int flag, int argc, char * const argv[])
> +{
> + int i, len;
> + char *name, *value, *s;
> + ENTRY e, *ep;
> +
> + name = argv[1];
> + value = argv[2];
> +
> + if (strchr(name, '=')) {
> + printf("## Error: illegal character '='"
> +"in variable name \"%s\"\n", name);
> + return 1;
> + }
> +
> + env_id++;
> + /*
> + * search if variable with this name already exists
> + */
> + e.key = name;
> + e.data = NULL;
> + hsearch_r(e, FIND, &ep, &env_htab);
> +
> + /*
> + * Perform requested checks. Notice how since we are overwriting
> + * a single variable, we need to set H_NOCLEAR
> + */
> + if (env_check_apply(name, ep ? ep->data : NULL, value, H_NOCLEAR)) {
> + debug("check function did not approve, refusing\n");
> + return 1;
> + }
> +
> /* Delete only ? */
> if (argc < 3 || argv[2] == NULL) {
> int rc = hdelete_r(name, &env_htab);
> @@ -337,34 +412,6 @@ int _do_env_set(int flag, int argc, char * const
> argv[]) return 1;
> }
>
> - /*
> - * Some variables should be updated when the corresponding
> - * entry in the environment is changed
> - */
> - if (strcmp(name, "ipaddr") == 0) {
> - char *s = argv[2]; /* always use only one arg */
> - char *e;
> - unsigned long addr;
> - bd->bi_ip_addr = 0;
> - for (addr = 0, i = 0; i < 4; ++i) {
> - ulong val = s ? simple_strtoul(s, &e, 10) : 0;
> - addr <<= 8;
> - addr |= val & 0xFF;
> - if (s)
> - s = *e ? e + 1 : e;
> - }
> - bd->bi_ip_addr = htonl(addr);
> - return 0;
> - } else if (strcmp(argv[1], "loadaddr") == 0) {
> - load_addr = simple_strtoul(argv[2], NULL, 16);
> - return 0;
> - }
> -#if defined(CONFIG_CMD_NET)
> - else if (strcmp(argv[1], "bootfile") == 0) {
> - copy_filename(BootFile, argv[2], sizeof(BootFile));
> - return 0;
> - }
> -#endif
> return 0;
> }
>
> @@ -886,7 +933,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> addr = (char *)ep->data;
> }
>
> - if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR) == 0) {
> + if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR,
> + 0, NULL, NULL) == 0) {
> error("Environment import failed: errno = %d\n", errno);
> return 1;
> }
> diff --git a/common/env_common.c b/common/env_common.c
> index 8a71096..7e2bb2f 100644
> --- a/common/env_common.c
> +++ b/common/env_common.c
> @@ -175,7 +175,8 @@ void set_default_env(const char *s)
> }
>
> if (himport_r(&env_htab, (char *)default_environment,
> - sizeof(default_environment), '\0', 0) == 0)
> + sizeof(default_environment), '\0', 0,
> + 0, NULL, NULL) == 0)
> error("Environment import failed: errno = %d\n", errno);
>
> gd->flags |= GD_FLG_ENV_READY;
> @@ -200,7 +201,8 @@ int env_import(const char *buf, int check)
> }
> }
>
> - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0)) {
> + if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0,
> + 0, NULL, NULL)) {
> gd->flags |= GD_FLG_ENV_READY;
> return 1;
> }
> diff --git a/include/environment.h b/include/environment.h
> index 3c145af..3a3e
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
Hi Gerlando,
On Mon, Dec 12, 2011 at 1:32 AM, Gerlando Falauto
wrote:
> On 12/07/2011 11:02 PM, Simon Glass wrote:
>
> [...]
>>>
>>> /*
>>> - * Set a new environment variable,
>>> - * or replace or delete an existing one.
>>> + * Performs consistency checking before setting, replacing,
>>> + * or deleting an environment variable, then (if successful)
>>> + * apply the changes to internals so to make them effective.
>>> + * Code for this function was taken out of _do_env_set(),
>>> + * which now calls it.
>>> + * Also called as a callback function by himport_r().
>>> + * Returns 0 in case of success, 1 in case of failure.
>>> + * When (flag& H_FORCE) is set, force overwriting of
>>> + * write-once variables.
>>
>>
>> can you word-wrap that to 72 columns perhaps?
>
>
> Sorry, I am little confused now. What is the maximum line length?
What I meant was that it seems like it you could put more characters
on each line, and then would take up less vertical space. You could
also use the doxygen @param formal to make it quicker for people to
read.
>
> [...]
>
>
>>> +/* Check whether variable name is amongst vars[] */
>>> +static int process_var(const char *name, int nvars, char * const vars[])
>>> +{
>>> + int i = 0;
>>
>>
>> blank line here.
>
>
> Thanks, I didn't know about this.
>
>
>> Can part of this function be #ifdefed to reduce code
>> size when the feature is not required?
>
>
> Uhm, I don't think so. This would be a common feature for selectively
> importing & setting back to default. Unless we want to #ifdef both of them.
> Personally, I would #ifdef neither.
OK, just a thought, it is up to you.
>
> [...]
>
>>
>> + if (!process_var(name, nvars, vars))
>> + continue;
>>
>> if (hdelete_r(name, htab) == 0)
>> debug("DELETE ERROR
>> ##\n");
>>
>> perhaps:
>>
>> if (process_var(name, nvars, vars)&&
>> hdelete_r(name, htab) == 0)
>> debug("DELETE ERROR ##\n");
>
>
>
> I think it's easier to read it the original way, and it should not make any
> difference as far as code size is concerned.
OK
>
>
>> himport_r() is getting a bit overloaded,
>
>
> Actually, I believe it makes no longer sense to have it called "_r", as it
> was the original reference to the function being recursively calleable (i.e.
> reentrant) as opposed to other versions which were not.
OK I'm not sure about that.
>
>
>> and it's a shame but I can't
>> think of an easy way to refactor it to reduce the number of args. In a
>> way you are adding a processing option and so you could put the three
>> extra args in a structure and pass a pointer to it (or NULL to skip
>> this feature). Not sure though...
>
>
> Can't think of any other way either, except maybe renaming it and
> re-implementing the shorter version as a wrapper around the newly named
> function. I had already done that, but there would be very few places where
> the old syntax would be kept, so it just didn't make much sense.
Packaging up a lot of zero arguments to a function does chew up code space.
>
>
>> Also, for me this patch adds 500 bytes. I wonder if more of the code
>> could made optional?
>
>
> Frankly, I'm surprised to hear this adds that much overhead.
> Actually, I can't see this increase in code size as you mention.
> What architecture are you referring to?
ARMv7. Ideally if your feature is optional it shouldn't add much size
when it is turned off.
> In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are
> surprisingly unchanged in size, even with debug #defined!
> Only place I can experience such growth is within unstripped u-boot ELF,
> which I believe shouldn't really matter... or should it?
As Wolfgang says, use your tool chain's size tool for this.
Regards,
Simon
>
> Best,
> Gerlando
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
Dear Gerlando Falauto, In message <4ee603d0.5010...@keymile.com> you wrote: > > I don't understand: why are you quoting this here and now? The chunk we > are referring to doesn't change a bit about the printable string. Please use proper indentation for it, even if it exceeds the line length then - although I wonder if the string actually makes any sense or is needed / useful as is. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de e-credibility: the non-guaranteeable likelihood that the electronic data you're seeing is genuine rather than somebody's made-up crap. - Karl Lehenbauer ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
On 12/12/2011 01:18 PM, Wolfgang Denk wrote: Dear Gerlando Falauto, I think it's easier to read it the original way, and it should not make any difference as far as code size is concerned. The Coding Style makes an explicit exception regarding the line length for user visible strings: 83 Statements longer than 80 columns will be broken into sensible chunks, unless 84 exceeding 80 columns significantly increases readability and does not hide 85 information. Descendants are always substantially shorter than the parent and 86 are placed substantially to the right. The same applies to function headers 87 with a long argument list. However, never break user-visible strings such as 88 printk messages, because that breaks the ability to grep for them. I don't understand: why are you quoting this here and now? The chunk we are referring to doesn't change a bit about the printable string. himport_r() is getting a bit overloaded, Actually, I believe it makes no longer sense to have it called "_r", as it was the original reference to the function being recursively calleable (i.e. reentrant) as opposed to other versions which were not. Has this changed? Of course it hasn't. But I don't see any point in keeping this notation anyway. Also, for me this patch adds 500 bytes. I wonder if more of the code could made optional? Frankly, I'm surprised to hear this adds that much overhead. Actually, I can't see this increase in code size as you mention. What architecture are you referring to? In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are surprisingly unchanged in size, even with debug #defined! Don't look at file size. Use the "size" command and compare code / data / bss sizes. Yep, sorry, I'll look into that to see what part is mostly reponsible for this. Best, Gerlando ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
Dear Gerlando Falauto,
In message <4ee5ca38.6090...@keymile.com> you wrote:
>
> > if (process_var(name, nvars, vars)&&
> > hdelete_r(name, htab) == 0)
> > debug("DELETE ERROR ##\n");
This is incorrect indentation.
> I think it's easier to read it the original way, and it should not make
> any difference as far as code size is concerned.
The Coding Style makes an explicit exception regarding the line length
for user visible strings:
83 Statements longer than 80 columns will be broken into sensible chunks,
unless
84 exceeding 80 columns significantly increases readability and does not hide
85 information. Descendants are always substantially shorter than the parent
and
86 are placed substantially to the right. The same applies to function headers
87 with a long argument list. However, never break user-visible strings such as
88 printk messages, because that breaks the ability to grep for them.
> > himport_r() is getting a bit overloaded,
>
> Actually, I believe it makes no longer sense to have it called "_r", as
> it was the original reference to the function being recursively
> calleable (i.e. reentrant) as opposed to other versions which were not.
Has this changed?
> > Also, for me this patch adds 500 bytes. I wonder if more of the code
> > could made optional?
>
> Frankly, I'm surprised to hear this adds that much overhead.
> Actually, I can't see this increase in code size as you mention.
> What architecture are you referring to?
> In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are
> surprisingly unchanged in size, even with debug #defined!
Don't look at file size. Use the "size" command and compare code /
data / bss sizes.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
As of 1992, they're called European Economic Community fries.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
On 12/07/2011 11:02 PM, Simon Glass wrote:
[...]
/*
- * Set a new environment variable,
- * or replace or delete an existing one.
+ * Performs consistency checking before setting, replacing,
+ * or deleting an environment variable, then (if successful)
+ * apply the changes to internals so to make them effective.
+ * Code for this function was taken out of _do_env_set(),
+ * which now calls it.
+ * Also called as a callback function by himport_r().
+ * Returns 0 in case of success, 1 in case of failure.
+ * When (flag& H_FORCE) is set, force overwriting of
+ * write-once variables.
can you word-wrap that to 72 columns perhaps?
Sorry, I am little confused now. What is the maximum line length?
[...]
+/* Check whether variable name is amongst vars[] */
+static int process_var(const char *name, int nvars, char * const vars[])
+{
+ int i = 0;
blank line here.
Thanks, I didn't know about this.
Can part of this function be #ifdefed to reduce code
size when the feature is not required?
Uhm, I don't think so. This would be a common feature for selectively
importing & setting back to default. Unless we want to #ifdef both of
them. Personally, I would #ifdef neither.
[...]
+ if (!process_var(name, nvars, vars))
+ continue;
if (hdelete_r(name, htab) == 0)
debug("DELETE ERROR
##\n");
perhaps:
if (process_var(name, nvars, vars)&&
hdelete_r(name, htab) == 0)
debug("DELETE ERROR ##\n");
I think it's easier to read it the original way, and it should not make
any difference as far as code size is concerned.
himport_r() is getting a bit overloaded,
Actually, I believe it makes no longer sense to have it called "_r", as
it was the original reference to the function being recursively
calleable (i.e. reentrant) as opposed to other versions which were not.
and it's a shame but I can't
think of an easy way to refactor it to reduce the number of args. In a
way you are adding a processing option and so you could put the three
extra args in a structure and pass a pointer to it (or NULL to skip
this feature). Not sure though...
Can't think of any other way either, except maybe renaming it and
re-implementing the shorter version as a wrapper around the newly named
function. I had already done that, but there would be very few places
where the old syntax would be kept, so it just didn't make much sense.
Also, for me this patch adds 500 bytes. I wonder if more of the code
could made optional?
Frankly, I'm surprised to hear this adds that much overhead.
Actually, I can't see this increase in code size as you mention.
What architecture are you referring to?
In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are
surprisingly unchanged in size, even with debug #defined!
Only place I can experience such growth is within unstripped u-boot ELF,
which I believe shouldn't really matter... or should it?
Best,
Gerlando
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
On Wednesday 07 December 2011 17:02:06 Simon Glass wrote: > Also, for me this patch adds 500 bytes. I wonder if more of the code > could made optional? yeah, it seems like these new params are only used when CONFIG_CMD_DEFAULTENV_VARS is defined. which means for most people i think, this is unused bloat :(. i wonder if we can do better somehow without having to resort to putting these behind #ifdef's. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes
Hi,
On Wed, Dec 7, 2011 at 5:30 AM, Gerlando Falauto
wrote:
> The logic of checking special parameters (e.g. baudrate, stdin, stdout,
> for a valid value and/or whether can be overwritten) and applying the
> new value to the running system is now all within a single function
> env_check_apply() which can be called whenever changes are made
> to the environment, no matter if by set, default or import.
Thanks for the rebase - I was able to try this out.
>
> With this patch env_check_apply() is only called by "env set",
> retaining previous behavior.
>
> Also allow for selectively importing/resetting variables.
>
> So add 3 new arguments to himport_r():
> o "nvars", "vars":, number and list of variables to take into account
> (0 means ALL)
>
> o "apply" callback function to check whether a variable can be
> overwritten, and possibly immediately apply the changes;
> when NULL, no check is performed.
>
> Signed-off-by: Gerlando Falauto
Tested-by: Simon Glass
> ---
> common/cmd_nvedit.c | 174
> +++--
> common/env_common.c | 6 +-
> include/environment.h | 7 ++
> include/search.h | 17 +-
> lib/hashtable.c | 43 -
> 5 files changed, 180 insertions(+), 67 deletions(-)
>
> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
> index 5995354..a31d413 100644
> --- a/common/cmd_nvedit.c
> +++ b/common/cmd_nvedit.c
> @@ -197,32 +197,23 @@ static int do_env_grep(cmd_tbl_t *cmdtp, int flag,
> #endif
>
> /*
> - * Set a new environment variable,
> - * or replace or delete an existing one.
> + * Performs consistency checking before setting, replacing,
> + * or deleting an environment variable, then (if successful)
> + * apply the changes to internals so to make them effective.
> + * Code for this function was taken out of _do_env_set(),
> + * which now calls it.
> + * Also called as a callback function by himport_r().
> + * Returns 0 in case of success, 1 in case of failure.
> + * When (flag & H_FORCE) is set, force overwriting of
> + * write-once variables.
can you word-wrap that to 72 columns perhaps?
> diff --git a/include/environment.h b/include/environment.h
> index 3c145af..3a3e6b8 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -193,6 +193,13 @@ void set_default_env(const char *s);
> /* Import from binary representation into hash table */
> int env_import(const char *buf, int check);
>
> +/*
> + * Check if variable "name" can be changed from oldval to newval,
> + * and if so, apply the changes (e.g. baudrate)
Can you document flag here also please?
> @@ -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);
can you document args also?
> +
> +/*
> * Family of hash table handling functions. The functions also
> * have reentrant counterparts ending with _r. The non-reentrant
> * functions all work on a signle internal hashing table.
> @@ -94,11 +101,19 @@ extern ssize_t hexport_r(struct hsearch_data *__htab,
> const char __sep, char **__resp, size_t __size,
> int argc, char * const argv[]);
>
> +/*
> + * nvars, vars: variables to import (nvars == 0 means all)
> + * apply_cb: callback function to check validity of the new argument,
> + * and possibly apply changes (NULL means accept everything)
> + */
What is vars? Is it a NULL terminated list of string pointers? Please
document. But actually you already have function comments in the C
file so I think you should either add your comments there or (better
in my view but I may be alone) move the comments to the header file.
> extern int himport_r(struct hsearch_data *__htab,
> const char *__env, size_t __size, const char __sep,
> - int __flag);
> + int __flag,
> + int nvars, char * const vars[],
> + apply_cb apply);
> 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[])
> +{
> + int i = 0;
blank line here. Can part of this function be #ifdefed to reduce code
size when the feature is not required?
> + /* 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"
