[U-Boot] [PATCH] net: Allow setenv to set net global variables

[Joe Hershberger]

Hi Chris,

On Mon, Jun 13, 2016 at 5:52 PM, Chris Packham
<Chris.Packham at 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 at 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 at 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 at 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 at alliedtelesis.co.nz>
>>>>>>> Reviewed-by: Hamish Martin <hamish.martin at alliedtelesis.co.nz>
>>>>>>> Reviewed-by: Chris Packham <chris.packham at 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