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