[U-Boot] [PATCH] net: Allow setenv to set net global variables
The patch fd3056337e6fcc introduces env callbacks to several of the net related env variables. These callbacks are responsible for updating the corresponding global variables internal to the net source code. However this behavior will be skipped if the source of the callbacks originated from setenv. This is based on the assumption that all current instances of setenv are invoked using the same global variables that the callback will eventually write to; therefore there is no need set them to the same value.
As setenv is a public interface this assumption may not always hold. In our usage case we implement a user facing menu system for configuration of networking parameters. This ultimately lead to calling setenv rather than through the traditional interactive command line parser do_env_set. Therefore, in our usage case, setenv can be called for an "interactive" case. Consequently, the early return for non-interactive invocation are now removed and any call to setenv will update the corresponding states internal to the net source code as expected.
Signed-off-by: Matthew Bright matthew.bright@alliedtelesis.co.nz Reviewed-by: Hamish Martin hamish.martin@alliedtelesis.co.nz Reviewed-by: Chris Packham chris.packham@alliedtelesis.co.nz --- net/net.c | 24 ------------------------ 1 file changed, 24 deletions(-)
diff --git a/net/net.c b/net/net.c index 1e1d23d..726b0f0 100644 --- a/net/net.c +++ b/net/net.c @@ -209,9 +209,6 @@ int __maybe_unused net_busy_flag; static int on_bootfile(const char *name, const char *value, enum env_op op, int flags) { - if (flags & H_PROGRAMMATIC) - return 0; - switch (op) { case env_op_create: case env_op_overwrite: @@ -229,9 +226,6 @@ U_BOOT_ENV_CALLBACK(bootfile, on_bootfile); static int on_ipaddr(const char *name, const char *value, enum env_op op, int flags) { - if (flags & H_PROGRAMMATIC) - return 0; - net_ip = string_to_ip(value);
return 0; @@ -241,9 +235,6 @@ U_BOOT_ENV_CALLBACK(ipaddr, on_ipaddr); static int on_gatewayip(const char *name, const char *value, enum env_op op, int flags) { - if (flags & H_PROGRAMMATIC) - return 0; - net_gateway = string_to_ip(value);
return 0; @@ -253,9 +244,6 @@ U_BOOT_ENV_CALLBACK(gatewayip, on_gatewayip); static int on_netmask(const char *name, const char *value, enum env_op op, int flags) { - if (flags & H_PROGRAMMATIC) - return 0; - net_netmask = string_to_ip(value);
return 0; @@ -265,9 +253,6 @@ U_BOOT_ENV_CALLBACK(netmask, on_netmask); static int on_serverip(const char *name, const char *value, enum env_op op, int flags) { - if (flags & H_PROGRAMMATIC) - return 0; - net_server_ip = string_to_ip(value);
return 0; @@ -277,9 +262,6 @@ U_BOOT_ENV_CALLBACK(serverip, on_serverip); static int on_nvlan(const char *name, const char *value, enum env_op op, int flags) { - if (flags & H_PROGRAMMATIC) - return 0; - net_native_vlan = string_to_vlan(value);
return 0; @@ -289,9 +271,6 @@ U_BOOT_ENV_CALLBACK(nvlan, on_nvlan); static int on_vlan(const char *name, const char *value, enum env_op op, int flags) { - if (flags & H_PROGRAMMATIC) - return 0; - net_our_vlan = string_to_vlan(value);
return 0; @@ -302,9 +281,6 @@ U_BOOT_ENV_CALLBACK(vlan, on_vlan); static int on_dnsip(const char *name, const char *value, enum env_op op, int flags) { - if (flags & H_PROGRAMMATIC) - return 0; - net_dns_server = string_to_ip(value);
return 0;
On Thu, Jun 9, 2016 at 8:40 PM, Matthew Bright matthew.bright@alliedtelesis.co.nz wrote:
The patch fd3056337e6fcc introduces env callbacks to several of the net related env variables. These callbacks are responsible for updating the corresponding global variables internal to the net source code. However this behavior will be skipped if the source of the callbacks originated from setenv. This is based on the assumption that all current instances of setenv are invoked using the same global variables that the callback will eventually write to; therefore there is no need set them to the same value.
As setenv is a public interface this assumption may not always hold. In our usage case we implement a user facing menu system for configuration of networking parameters. This ultimately lead to calling setenv rather than through the traditional interactive command line parser do_env_set. Therefore, in our usage case, setenv can be called for an "interactive" case. Consequently, the early return for non-interactive invocation are now removed and any call to setenv will update the corresponding states internal to the net source code as expected.
Signed-off-by: Matthew Bright matthew.bright@alliedtelesis.co.nz Reviewed-by: Hamish Martin hamish.martin@alliedtelesis.co.nz Reviewed-by: Chris Packham chris.packham@alliedtelesis.co.nz
net/net.c | 24 ------------------------ 1 file changed, 24 deletions(-)
diff --git a/net/net.c b/net/net.c index 1e1d23d..726b0f0 100644 --- a/net/net.c +++ b/net/net.c @@ -209,9 +209,6 @@ int __maybe_unused net_busy_flag; static int on_bootfile(const char *name, const char *value, enum env_op op, int flags) {
if (flags & H_PROGRAMMATIC)return 0;
Why can't you just change your menu to call the API that is interactive instead of setenv?
Thanks, -Joe
Hi Joe,
On 06/11/2016 03:56 AM, Joe Hershberger wrote:
On Thu, Jun 9, 2016 at 8:40 PM, Matthew Bright matthew.bright@alliedtelesis.co.nz wrote:
The patch fd3056337e6fcc introduces env callbacks to several of the net related env variables. These callbacks are responsible for updating the corresponding global variables internal to the net source code. However this behavior will be skipped if the source of the callbacks originated from setenv. This is based on the assumption that all current instances of setenv are invoked using the same global variables that the callback will eventually write to; therefore there is no need set them to the same value.
As setenv is a public interface this assumption may not always hold. In our usage case we implement a user facing menu system for configuration of networking parameters. This ultimately lead to calling setenv rather than through the traditional interactive command line parser do_env_set. Therefore, in our usage case, setenv can be called for an "interactive" case. Consequently, the early return for non-interactive invocation are now removed and any call to setenv will update the corresponding states internal to the net source code as expected.
Signed-off-by: Matthew Bright matthew.bright@alliedtelesis.co.nz Reviewed-by: Hamish Martin hamish.martin@alliedtelesis.co.nz Reviewed-by: Chris Packham chris.packham@alliedtelesis.co.nz
net/net.c | 24 ------------------------ 1 file changed, 24 deletions(-)
diff --git a/net/net.c b/net/net.c index 1e1d23d..726b0f0 100644 --- a/net/net.c +++ b/net/net.c @@ -209,9 +209,6 @@ int __maybe_unused net_busy_flag; static int on_bootfile(const char *name, const char *value, enum env_op op, int flags) {
if (flags & H_PROGRAMMATIC)return 0;Why can't you just change your menu to call the API that is interactive instead of setenv?
Which API are you referring to? _do_env_set() is static so the only public api would be run_command("setenv ipaddr ...") or have I missed something?
Hi Chris,
On Sun, Jun 12, 2016 at 3:58 PM, Chris Packham Chris.Packham@alliedtelesis.co.nz wrote:
Hi Joe,
On 06/11/2016 03:56 AM, Joe Hershberger wrote:
On Thu, Jun 9, 2016 at 8:40 PM, Matthew Bright matthew.bright@alliedtelesis.co.nz wrote:
The patch fd3056337e6fcc introduces env callbacks to several of the net related env variables. These callbacks are responsible for updating the corresponding global variables internal to the net source code. However this behavior will be skipped if the source of the callbacks originated from setenv. This is based on the assumption that all current instances of setenv are invoked using the same global variables that the callback will eventually write to; therefore there is no need set them to the same value.
As setenv is a public interface this assumption may not always hold. In our usage case we implement a user facing menu system for configuration of networking parameters. This ultimately lead to calling setenv rather than through the traditional interactive command line parser do_env_set. Therefore, in our usage case, setenv can be called for an "interactive" case. Consequently, the early return for non-interactive invocation are now removed and any call to setenv will update the corresponding states internal to the net source code as expected.
Signed-off-by: Matthew Bright matthew.bright@alliedtelesis.co.nz Reviewed-by: Hamish Martin hamish.martin@alliedtelesis.co.nz Reviewed-by: Chris Packham chris.packham@alliedtelesis.co.nz
net/net.c | 24 ------------------------ 1 file changed, 24 deletions(-)
diff --git a/net/net.c b/net/net.c index 1e1d23d..726b0f0 100644 --- a/net/net.c +++ b/net/net.c @@ -209,9 +209,6 @@ int __maybe_unused net_busy_flag; static int on_bootfile(const char *name, const char *value, enum env_op op, int flags) {
if (flags & H_PROGRAMMATIC)return 0;Why can't you just change your menu to call the API that is interactive instead of setenv?
Which API are you referring to? _do_env_set() is static so the only public api would be run_command("setenv ipaddr ...") or have I missed something?
Yes, that's what I was referring to.
Another option would be to add an explicit function that provides this directly. Maybe even make a generic version that accepts a flags parameter, then implement the existing function as a call to this new function which passes in a "programmatic" flag.
-Joe
On 06/14/2016 06:34 AM, Joe Hershberger wrote:
Hi Chris,
On Sun, Jun 12, 2016 at 3:58 PM, Chris Packham Chris.Packham@alliedtelesis.co.nz wrote:
Hi Joe,
On 06/11/2016 03:56 AM, Joe Hershberger wrote:
On Thu, Jun 9, 2016 at 8:40 PM, Matthew Bright matthew.bright@alliedtelesis.co.nz wrote:
The patch fd3056337e6fcc introduces env callbacks to several of the net related env variables. These callbacks are responsible for updating the corresponding global variables internal to the net source code. However this behavior will be skipped if the source of the callbacks originated from setenv. This is based on the assumption that all current instances of setenv are invoked using the same global variables that the callback will eventually write to; therefore there is no need set them to the same value.
As setenv is a public interface this assumption may not always hold. In our usage case we implement a user facing menu system for configuration of networking parameters. This ultimately lead to calling setenv rather than through the traditional interactive command line parser do_env_set. Therefore, in our usage case, setenv can be called for an "interactive" case. Consequently, the early return for non-interactive invocation are now removed and any call to setenv will update the corresponding states internal to the net source code as expected.
Signed-off-by: Matthew Bright matthew.bright@alliedtelesis.co.nz Reviewed-by: Hamish Martin hamish.martin@alliedtelesis.co.nz Reviewed-by: Chris Packham chris.packham@alliedtelesis.co.nz
net/net.c | 24 ------------------------ 1 file changed, 24 deletions(-)
diff --git a/net/net.c b/net/net.c index 1e1d23d..726b0f0 100644 --- a/net/net.c +++ b/net/net.c @@ -209,9 +209,6 @@ int __maybe_unused net_busy_flag; static int on_bootfile(const char *name, const char *value, enum env_op op, int flags) {
if (flags & H_PROGRAMMATIC)return 0;Why can't you just change your menu to call the API that is interactive instead of setenv?
Which API are you referring to? _do_env_set() is static so the only public api would be run_command("setenv ipaddr ...") or have I missed something?
Yes, that's what I was referring to.
Another option would be to add an explicit function that provides this directly. Maybe even make a generic version that accepts a flags parameter, then implement the existing function as a call to this new function which passes in a "programmatic" flag.
That's what I was thinking. Because setenv is one of the exported functions for standalone applications I was wondering if instead of setenv() passing H_PROGRAMMATIC we add prog_setenv() (naming things is hard) for the net use-case since that is the only thing that currently checks H_PROGRAMMATIC.
Hi Chris,
On Mon, Jun 13, 2016 at 4:13 PM, Chris Packham Chris.Packham@alliedtelesis.co.nz wrote:
On 06/14/2016 06:34 AM, Joe Hershberger wrote:
Hi Chris,
On Sun, Jun 12, 2016 at 3:58 PM, Chris Packham Chris.Packham@alliedtelesis.co.nz wrote:
Hi Joe,
On 06/11/2016 03:56 AM, Joe Hershberger wrote:
On Thu, Jun 9, 2016 at 8:40 PM, Matthew Bright matthew.bright@alliedtelesis.co.nz wrote:
The patch fd3056337e6fcc introduces env callbacks to several of the net related env variables. These callbacks are responsible for updating the corresponding global variables internal to the net source code. However this behavior will be skipped if the source of the callbacks originated from setenv. This is based on the assumption that all current instances of setenv are invoked using the same global variables that the callback will eventually write to; therefore there is no need set them to the same value.
As setenv is a public interface this assumption may not always hold. In our usage case we implement a user facing menu system for configuration of networking parameters. This ultimately lead to calling setenv rather than through the traditional interactive command line parser do_env_set. Therefore, in our usage case, setenv can be called for an "interactive" case. Consequently, the early return for non-interactive invocation are now removed and any call to setenv will update the corresponding states internal to the net source code as expected.
Signed-off-by: Matthew Bright matthew.bright@alliedtelesis.co.nz Reviewed-by: Hamish Martin hamish.martin@alliedtelesis.co.nz Reviewed-by: Chris Packham chris.packham@alliedtelesis.co.nz
net/net.c | 24 ------------------------ 1 file changed, 24 deletions(-)
diff --git a/net/net.c b/net/net.c index 1e1d23d..726b0f0 100644 --- a/net/net.c +++ b/net/net.c @@ -209,9 +209,6 @@ int __maybe_unused net_busy_flag; static int on_bootfile(const char *name, const char *value, enum env_op op, int flags) {
if (flags & H_PROGRAMMATIC)return 0;Why can't you just change your menu to call the API that is interactive instead of setenv?
Which API are you referring to? _do_env_set() is static so the only public api would be run_command("setenv ipaddr ...") or have I missed something?
Yes, that's what I was referring to.
Another option would be to add an explicit function that provides this directly. Maybe even make a generic version that accepts a flags parameter, then implement the existing function as a call to this new function which passes in a "programmatic" flag.
That's what I was thinking. Because setenv is one of the exported functions for standalone applications I was wondering if instead of setenv() passing H_PROGRAMMATIC we add prog_setenv() (naming things is hard) for the net use-case since that is the only thing that currently checks H_PROGRAMMATIC.
That might be OK. The only reservation I have about it is that the setenv() function is generally a programmatic operation since only C code can get to it. Only in the case where you are implementing some more complex interaction (like your menu) is it not actually programmatic. I just worry about it being misleading in the future.
-Joe
On 06/14/2016 10:19 AM, Joe Hershberger wrote:
Hi Chris,
On Mon, Jun 13, 2016 at 4:13 PM, Chris Packham Chris.Packham@alliedtelesis.co.nz wrote:
On 06/14/2016 06:34 AM, Joe Hershberger wrote:
Hi Chris,
On Sun, Jun 12, 2016 at 3:58 PM, Chris Packham Chris.Packham@alliedtelesis.co.nz wrote:
Hi Joe,
On 06/11/2016 03:56 AM, Joe Hershberger wrote:
On Thu, Jun 9, 2016 at 8:40 PM, Matthew Bright matthew.bright@alliedtelesis.co.nz wrote:
The patch fd3056337e6fcc introduces env callbacks to several of the net related env variables. These callbacks are responsible for updating the corresponding global variables internal to the net source code. However this behavior will be skipped if the source of the callbacks originated from setenv. This is based on the assumption that all current instances of setenv are invoked using the same global variables that the callback will eventually write to; therefore there is no need set them to the same value.
As setenv is a public interface this assumption may not always hold. In our usage case we implement a user facing menu system for configuration of networking parameters. This ultimately lead to calling setenv rather than through the traditional interactive command line parser do_env_set. Therefore, in our usage case, setenv can be called for an "interactive" case. Consequently, the early return for non-interactive invocation are now removed and any call to setenv will update the corresponding states internal to the net source code as expected.
Signed-off-by: Matthew Bright matthew.bright@alliedtelesis.co.nz Reviewed-by: Hamish Martin hamish.martin@alliedtelesis.co.nz Reviewed-by: Chris Packham chris.packham@alliedtelesis.co.nz
net/net.c | 24 ------------------------ 1 file changed, 24 deletions(-)diff --git a/net/net.c b/net/net.c index 1e1d23d..726b0f0 100644 --- a/net/net.c +++ b/net/net.c @@ -209,9 +209,6 @@ int __maybe_unused net_busy_flag; static int on_bootfile(const char *name, const char *value, enum env_op op, int flags) {
if (flags & H_PROGRAMMATIC)return 0;Why can't you just change your menu to call the API that is interactive instead of setenv?
Which API are you referring to? _do_env_set() is static so the only public api would be run_command("setenv ipaddr ...") or have I missed something?
Yes, that's what I was referring to.
Another option would be to add an explicit function that provides this directly. Maybe even make a generic version that accepts a flags parameter, then implement the existing function as a call to this new function which passes in a "programmatic" flag.
That's what I was thinking. Because setenv is one of the exported functions for standalone applications I was wondering if instead of setenv() passing H_PROGRAMMATIC we add prog_setenv() (naming things is hard) for the net use-case since that is the only thing that currently checks H_PROGRAMMATIC.
That might be OK. The only reservation I have about it is that the setenv() function is generally a programmatic operation since only C code can get to it. Only in the case where you are implementing some more complex interaction (like your menu) is it not actually programmatic. I just worry about it being misleading in the future.
Agreed. My initial reaction was that our menu should be treated like H_INTERACTIVE but there wasn't an easy way to achieve this.
Do you have any feel for the direction of H_PROGRAMMATIC is going? Are we going to see more environment variables in other parts of the code that will get similar treatment.
Hi Chris,
On Mon, Jun 13, 2016 at 5:52 PM, Chris Packham Chris.Packham@alliedtelesis.co.nz wrote:
On 06/14/2016 10:19 AM, Joe Hershberger wrote:
Hi Chris,
On Mon, Jun 13, 2016 at 4:13 PM, Chris Packham Chris.Packham@alliedtelesis.co.nz wrote:
On 06/14/2016 06:34 AM, Joe Hershberger wrote:
Hi Chris,
On Sun, Jun 12, 2016 at 3:58 PM, Chris Packham Chris.Packham@alliedtelesis.co.nz wrote:
Hi Joe,
On 06/11/2016 03:56 AM, Joe Hershberger wrote:
On Thu, Jun 9, 2016 at 8:40 PM, Matthew Bright matthew.bright@alliedtelesis.co.nz wrote: > The patch fd3056337e6fcc introduces env callbacks to several of the net > related env variables. These callbacks are responsible for updating the > corresponding global variables internal to the net source code. However > this behavior will be skipped if the source of the callbacks originated > from setenv. This is based on the assumption that all current instances > of setenv are invoked using the same global variables that the callback > will eventually write to; therefore there is no need set them to the > same value. > > As setenv is a public interface this assumption may not always hold. In > our usage case we implement a user facing menu system for configuration > of networking parameters. This ultimately lead to calling setenv rather > than through the traditional interactive command line parser do_env_set. > Therefore, in our usage case, setenv can be called for an "interactive" > case. Consequently, the early return for non-interactive invocation are > now removed and any call to setenv will update the corresponding states > internal to the net source code as expected. > > Signed-off-by: Matthew Bright matthew.bright@alliedtelesis.co.nz > Reviewed-by: Hamish Martin hamish.martin@alliedtelesis.co.nz > Reviewed-by: Chris Packham chris.packham@alliedtelesis.co.nz > --- > net/net.c | 24 ------------------------ > 1 file changed, 24 deletions(-) > > diff --git a/net/net.c b/net/net.c > index 1e1d23d..726b0f0 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -209,9 +209,6 @@ int __maybe_unused net_busy_flag; > static int on_bootfile(const char *name, const char *value, enum env_op op, > int flags) > { > - if (flags & H_PROGRAMMATIC) > - return 0; > -
Why can't you just change your menu to call the API that is interactive instead of setenv?
Which API are you referring to? _do_env_set() is static so the only public api would be run_command("setenv ipaddr ...") or have I missed something?
Yes, that's what I was referring to.
Another option would be to add an explicit function that provides this directly. Maybe even make a generic version that accepts a flags parameter, then implement the existing function as a call to this new function which passes in a "programmatic" flag.
That's what I was thinking. Because setenv is one of the exported functions for standalone applications I was wondering if instead of setenv() passing H_PROGRAMMATIC we add prog_setenv() (naming things is hard) for the net use-case since that is the only thing that currently checks H_PROGRAMMATIC.
That might be OK. The only reservation I have about it is that the setenv() function is generally a programmatic operation since only C code can get to it. Only in the case where you are implementing some more complex interaction (like your menu) is it not actually programmatic. I just worry about it being misleading in the future.
Agreed. My initial reaction was that our menu should be treated like H_INTERACTIVE but there wasn't an easy way to achieve this.
Do you have any feel for the direction of H_PROGRAMMATIC is going? Are we going to see more environment variables in other parts of the code that will get similar treatment.
Given that I implemented the code in question, I can't say I can give an unbiased opinion about the direction. I would tend toward using this same paradigm in other places. :)
I can prolly send an RFC tomorrow that shows what I have in mind for addressing this.
Cheers, -Joe
participants (3)
-
Chris Packham -
Joe Hershberger -
Matthew Bright