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.