[U-Boot] Setting ipaddr via setenv() - H_INTERACTIVE vs H_PROGRAMMATIC

[Joe Hershberger]

Hi Stefan,

On Wed, Mar 2, 2016 at 12:21 AM, Stefan Roese <sr at denx.de> wrote:
> Hi Joe,
>
> (adding Tom to Cc as this seems to be a fundamental issue)
>
> On 01.03.2016 20:40, Joe Hershberger wrote:
>> On Tue, Mar 1, 2016 at 4:35 AM, Stefan Roese <sr at denx.de> wrote:
>>> Hi Joe,
>>>
>>> I'm currently stumbling over a problem in some board specific code
>>> (not in mainline yet), that sets the "ipaddr" env variable via
>>> setenv(). But as I now noticed, the callback on_ipaddr() doesn't
>>> set net_ip to this new value. As flags has H_PROGRAMMATIC set and
>>> op_ipaddr() skips setting this value in this case.
>>>
>>> I fail to see why we need this check in on_ipaddr(). Could you
>>> please explain why this is needed? Why would someone use setenv()
>>> for the "ipaddr" and not want to also update the net_ip value.
>>> This results in the "ipaddr" value being updated but net_ip
>>> still being configured to the old value.
>>
>> The purpose is that programmatic accesses may write directly to the
>> net_ip variable, but the user on the console cannot. This is because
>> the "programmatic" accesses to these variables is expected to be
>> things like the dhcp command and the linklocal command after
>> successful netloop.
>
> Please correct me, but it seems to be implemented the other way around.
> If H_PROGRAMMATIC is set (called via setenv()) the net variables
> (net_ip...) will *not* be set:
>
> 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;
> }
>
>> You can see this call in the netboot_update_env()
>> function in cmd/net.c.
>
> netboot_update_env() does not update the internal variables. At least
> not in my tests. op_ipaddr() returns directly after the "flags" check
> (see above).

That's exactly the point. It doesn't set the internal variables
because the source *is* the internal variables.

> I've checked how "dhcp" works. And here the variables are updated
> in store_net_params() via explicit net_copy_ip() calls.
>
>>> Or if this is really needed, what is the correct way to update
>>> the ipaddr (env variable and net_ip value) from the U-Boot code?
>>
>> You can work around it in a similar way to what I did in the initial
>> change (94b467b14ed908c89a0780256e89b375aa3cf3ef - env: Distinguish
>> finer between source of env change). The do_env_edit function used to
>> call setenv, but I changed it to call as though it was interactive.
>> You could do the same thing in your board driver.
>
> Again, my findings are that this is implemented the other way around.
> setenv from the user via the command "setenv ipaddr ..." will update
> the net_ip variable. And code that calls setenv() will not.
>
> I still fails to see why one of both should not update the net_ip
> (and others) variable. From my understanding setenv() from code and
> "setenv ipaddr ..." from the prompt should update the internal
> variables.
>
> Thanks,
> Stefan
>