* [U-Boot] Setting ipaddr via setenv() - H_INTERACTIVE vs H_PROGRAMMATIC @ 2016-03-01 10:35 Stefan Roese 2016-03-01 19:40 ` Joe Hershberger 0 siblings, 1 reply; 6+ messages in thread From: Stefan Roese @ 2016-03-01 10:35 UTC (permalink / raw) To: u-boot 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. 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? Thanks, Stefan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] Setting ipaddr via setenv() - H_INTERACTIVE vs H_PROGRAMMATIC 2016-03-01 10:35 [U-Boot] Setting ipaddr via setenv() - H_INTERACTIVE vs H_PROGRAMMATIC Stefan Roese @ 2016-03-01 19:40 ` Joe Hershberger 2016-03-02 6:21 ` Stefan Roese 0 siblings, 1 reply; 6+ messages in thread From: Joe Hershberger @ 2016-03-01 19:40 UTC (permalink / raw) To: u-boot Hi Stefan, On Tue, Mar 1, 2016 at 4:35 AM, Stefan Roese <sr@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. You can see this call in the netboot_update_env() function in cmd/net.c. > 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. Cheers, -Joe ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] Setting ipaddr via setenv() - H_INTERACTIVE vs H_PROGRAMMATIC 2016-03-01 19:40 ` Joe Hershberger @ 2016-03-02 6:21 ` Stefan Roese 2016-03-02 6:46 ` Joe Hershberger 0 siblings, 1 reply; 6+ messages in thread From: Stefan Roese @ 2016-03-02 6:21 UTC (permalink / raw) To: u-boot 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@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). 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] Setting ipaddr via setenv() - H_INTERACTIVE vs H_PROGRAMMATIC 2016-03-02 6:21 ` Stefan Roese @ 2016-03-02 6:46 ` Joe Hershberger 2016-03-02 7:01 ` Stefan Roese 0 siblings, 1 reply; 6+ messages in thread From: Joe Hershberger @ 2016-03-02 6:46 UTC (permalink / raw) To: u-boot Hi Stefan, On Wed, Mar 2, 2016 at 12:21 AM, Stefan Roese <sr@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@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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] Setting ipaddr via setenv() - H_INTERACTIVE vs H_PROGRAMMATIC 2016-03-02 6:46 ` Joe Hershberger @ 2016-03-02 7:01 ` Stefan Roese 2016-03-03 1:58 ` Joe Hershberger 0 siblings, 1 reply; 6+ messages in thread From: Stefan Roese @ 2016-03-02 7:01 UTC (permalink / raw) To: u-boot Hi Joe, On 02.03.2016 07:46, Joe Hershberger wrote: > On Wed, Mar 2, 2016 at 12:21 AM, Stefan Roese <sr@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@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. Now I understand what you mean. The PROGRAMMATIC approach should set the variables directly. I'll set the net_ip variable directly from the board specific code then. But again, why exactly do we need to distinguish between this interactive and programmatic access? Why not set the internal variables every time the special env variables are set (interactively and programmatically)? Thanks, Stefan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] Setting ipaddr via setenv() - H_INTERACTIVE vs H_PROGRAMMATIC 2016-03-02 7:01 ` Stefan Roese @ 2016-03-03 1:58 ` Joe Hershberger 0 siblings, 0 replies; 6+ messages in thread From: Joe Hershberger @ 2016-03-03 1:58 UTC (permalink / raw) To: u-boot On Wed, Mar 2, 2016 at 1:01 AM, Stefan Roese <sr@denx.de> wrote: > Hi Joe, > > > On 02.03.2016 07:46, Joe Hershberger wrote: >> >> On Wed, Mar 2, 2016 at 12:21 AM, Stefan Roese <sr@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@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. > > > Now I understand what you mean. The PROGRAMMATIC approach should set > the variables directly. I'll set the net_ip variable directly from > the board specific code then. > > But again, why exactly do we need to distinguish between this > interactive and programmatic access? Why not set the internal variables > every time the special env variables are set (interactively and > programmatically)? It was simply for the reason above. I think in this case, it is writing the variable with the previous value and would be harmless. In general it is to be able to avoid infinite loops if the on_blah handler is calling a helper that updates the env, etc. -Joe ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-03 1:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-01 10:35 [U-Boot] Setting ipaddr via setenv() - H_INTERACTIVE vs H_PROGRAMMATIC Stefan Roese 2016-03-01 19:40 ` Joe Hershberger 2016-03-02 6:21 ` Stefan Roese 2016-03-02 6:46 ` Joe Hershberger 2016-03-02 7:01 ` Stefan Roese 2016-03-03 1:58 ` Joe Hershberger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox