Improve env var handling for net stack

Enable some additional ENV commands in sandbox to aid in build testing and run testing.

Signed-off-by Joe Hershberger <[email protected]>

---

 include/configs/sandbox.h | 5 +++++
 1 file changed, 5 insertions(+)

Having this as a Kconfig allows it to be a dependent feature.

Signed-off-by Joe Hershberger <[email protected]>

---

 configs/acadia_defconfig           | 1 +
 configs/bamboo_defconfig           | 1 +
 configs/bubinga_defconfig          | 1 +
 configs/canyonlands_defconfig      | 1 +
 configs/dlvision-10g_defconfig     | 1 +
 configs/dlvision_defconfig         | 1 +
 configs/ebony_defconfig            | 1 +
 configs/gdppc440etx_defconfig      | 1 +
 configs/icon_defconfig             | 1 +
 configs/intip_defconfig            | 1 +
 configs/io64_defconfig             | 1 +
 configs/io_defconfig               | 1 +
 configs/iocon_defconfig            | 1 +
 configs/katmai_defconfig           | 1 +
 configs/kilauea_defconfig          | 1 +
 configs/luan_defconfig             | 1 +
 configs/m28evk_defconfig           | 1 +
 configs/m53evk_defconfig           | 1 +
 configs/makalu_defconfig           | 1 +
 configs/neo_defconfig              | 1 +
 configs/novena_defconfig           | 1 +
 configs/ocotea_defconfig           | 1 +
 configs/redwood_defconfig          | 1 +
 configs/sequoia_defconfig          | 1 +
 configs/socfpga_arria5_defconfig   | 1 +
 configs/socfpga_cyclone5_defconfig | 1 +
 configs/t3corp_defconfig           | 1 +
 configs/taihu_defconfig            | 1 +
 configs/taishan_defconfig          | 1 +
 configs/walnut_defconfig           | 1 +
 configs/yosemite_defconfig         | 1 +
 configs/yucca_defconfig            | 1 +

 include/configs/amcc-common.h      | 1 -
 include/configs/m28evk.h           | 1 -
 include/configs/m53evk.h           | 1 -
 include/configs/novena.h           | 1 -
 include/configs/socfpga_arria5.h   | 1 -
 include/configs/socfpga_cyclone5.h | 1 -
 lib/Kconfig                        | 8 ++++++++
 39 files changed, 40 insertions(+), 6 deletions(-)

Having this as a Kconfig allows it to be a dependent feature.

Signed-off-by Joe Hershberger <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Quoted 4 lines · Joe Hershberger

Having this as a Kconfig allows it to be a dependent feature.

Signed-off-by Joe Hershberger <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Applied to u-boot/master, thanks!

Hi Tom,

Quoted 2 lines · Tom Rini

On Tue, Apr 21, 2015 at 05:02:42PM -0500, Joe Hershberger wrote:

Quoted 6 lines

Quoted 4 lines

Having this as a Kconfig allows it to be a dependent feature.

Signed-off-by Joe Hershberger <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Applied to u-boot/master, thanks!

What about the rest of the series (v4)?

Enable regex support on sandbox.

Signed-off-by Joe Hershberger <[email protected]>

---

 configs/sandbox_defconfig | 1 +
 1 file changed, 1 insertion(+)

This function returned numbers for error codes. Change them to error codes.

Signed-off-by Joe Hershberger <[email protected]>

---

 common/env_attr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

The logic to find the whole matching name was split needlessly between the reverse_strstr function and its caller. Fully contain it to make the interface for calling it more consistent.

Signed-off-by Joe Hershberger <[email protected]>

---

 common/env_attr.c | 79 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 41 insertions(+), 38 deletions(-)

common/env_attr.c +41 -39

index e791f44..d266142 100644
@@ -109,33 +109,55 @@ int env_attr_walk(const char *attr_list,
 }
 
 /*
- * Search for the last matching string in another string with the option to
- * start looking at a certain point (i.e. ignore anything beyond that point).
+ * Search for the last exactly matching name in an attribute list
  */
-static char *reverse_strstr(const char *searched, const char *search_for,
-       const char *searched_start)
+static int reverse_name_search(const char *searched, const char *search_for,
+       const char **result)
 {
-       char *result = NULL;
+       int result_size = 0;
+       const char *cur_searched = searched;
+
+       if (result)
+               *result = NULL;
 
        if (*search_for == '\0')
                return (char *)searched;
 
        for (;;) {
-               char *match = strstr(searched, search_for);
-
-               /*
-                * Stop looking if no new match is found or looking past the
-                * searched_start pointer
-                */
-               if (match == NULL || (searched_start != NULL &&
-                   match + strlen(search_for) > searched_start))
+               const char *match = strstr(cur_searched, search_for);
+               const char *prevch;
+               const char *nextch;
+
+               /* Stop looking if no new match is found */
+               if (match == NULL)
                        break;
 
-               result = match;
-               searched = match + 1;
+               prevch = match - 1;
+               nextch = match + strlen(search_for);
+
+               /* Skip spaces */
+               while (*prevch == ' ')
+                       prevch--;
+               while (*nextch == ' ')
+                       nextch++;
+
+               /* Start looking past the current match so last is found */
+               cur_searched = match + 1;
+
+               /* Check for an exact match */
+               if (match != searched &&
+                   *prevch != ENV_ATTR_LIST_DELIM)
+                       continue;
+               if (*nextch != ENV_ATTR_SEP &&
+                   *nextch != ENV_ATTR_LIST_DELIM &&
+                   *nextch != '\0')
+                       continue;
+
+               *result = match;
+               result_size = strlen(search_for);
        }
 
-       return result;
+       return result_size;
 }
 
 /*

@@ -145,6 +167,7 @@ static char *reverse_strstr(const char *searched, const char *search_for,
 int env_attr_lookup(const char *attr_list, const char *name, char *attributes)
 {
        const char *entry = NULL;
+       int entry_len;
 
        if (!attributes)
                /* bad parameter */

@@ -153,32 +176,12 @@ int env_attr_lookup(const char *attr_list, const char *name, char *attributes)
                /* list not found */
                return -EINVAL;
 
-       entry = reverse_strstr(attr_list, name, NULL);
-       while (entry != NULL) {
-               const char *prevch = entry - 1;
-               const char *nextch = entry + strlen(name);
-
-               /* Skip spaces */
-               while (*prevch == ' ')
-                       prevch--;
-               while (*nextch == ' ')
-                       nextch++;
-
-               /* check for an exact match */
-               if ((entry == attr_list ||
-                    *prevch == ENV_ATTR_LIST_DELIM) &&
-                   (*nextch == ENV_ATTR_SEP ||
-                    *nextch == ENV_ATTR_LIST_DELIM ||
-                    *nextch == '\0'))
-                       break;
-
-               entry = reverse_strstr(attr_list, name, entry);
-       }
+       entry_len = reverse_name_search(attr_list, name, &entry);
        if (entry != NULL) {
                int len;
 
                /* skip the name */
-               entry += strlen(name);
+               entry += entry_len;
                /* skip spaces */
                while (*entry == ' ')
                        entry++;
--

1.7.11.5

Hi Joe,

Quoted 7 lines · Joe Hershberger

The logic to find the whole matching name was split needlessly between the reverse_strstr function and its caller. Fully contain it to make the interface for calling it more consistent.

Signed-off-by Joe Hershberger <[email protected]>

---

Quoted 3 lines

 common/env_attr.c | 79 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 41 insertions(+), 38 deletions(-)

You could perhaps add some environment tests in test/ for this function, or access it through getenv(), etc.

Hi Simon,

Quoted 10 lines · Simon Glass

Hi Joe,

Quoted 7 lines · Joe Hershberger

The logic to find the whole matching name was split needlessly between the reverse_strstr function and its caller. Fully contain it to make the interface for calling it more consistent.

Signed-off-by Joe Hershberger <[email protected]>

---

Quoted 6 lines

Quoted 3 lines

 common/env_attr.c | 79 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 41 insertions(+), 38 deletions(-)

You could perhaps add some environment tests in test/ for this function, or access it through getenv(), etc.

I'll look into adding some unit tests for the env stuff.

Hi Simon,

Quoted 17 lines · Joe Hershberger

Hi Simon,

Quoted 14 lines · Simon Glass

Hi Joe,

Quoted 10 lines · Joe Hershberger

The logic to find the whole matching name was split needlessly between the reverse_strstr function and its caller. Fully contain it to make the interface for calling it more consistent.

Signed-off-by Joe Hershberger <[email protected]>

---

 common/env_attr.c | 79 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 41 insertions(+), 38 deletions(-)

Quoted 4 lines

Quoted 2 lines

You could perhaps add some environment tests in test/ for this function, or access it through getenv(), etc.

I'll look into adding some unit tests for the env stuff.

I'd like to reuse a bit of the unit test code from DM tests... do you have any plans to generalize some of it? Or should I take a crack at some of it (that I want to reuse)?

In some cases it can be helpful to have context in the callback about the calling situation. This is needed for following patches.

Signed-off-by Joe Hershberger <[email protected]>

---

common/env_attr.c +3 -2

index d266142..f0bf504 100644
@@ -26,7 +26,8 @@
  *     list = entry[,list]
  */
 int env_attr_walk(const char *attr_list,
-       int (*callback)(const char *name, const char *attributes))
+       int (*callback)(const char *name, const char *attributes, void *priv),
+       void *priv)
 {
        const char *entry, *entry_end;
        char *name, *attributes;

@@ -93,7 +94,7 @@ int env_attr_walk(const char *attr_list,
                        if (strlen(name) != 0) {
                                int retval = 0;
 
-                               retval = callback(name, attributes);
+                               retval = callback(name, attributes, priv);
                                if (retval) {
                                        free(entry_cpy);
                                        return retval;

Hi All,

Quoted 5 lines · Joe Hershberger

In some cases it can be helpful to have context in the callback about the calling situation. This is needed for following patches.

Signed-off-by Joe Hershberger <[email protected]>

---

8<--snip-->8

Quoted 24 lines

common/env_attr.c +3 -2

index d266142..f0bf504 100644
@@ -26,7 +26,8 @@
  *     list = entry[,list]
  */
 int env_attr_walk(const char *attr_list,
-       int (*callback)(const char *name, const char *attributes))
+       int (*callback)(const char *name, const char *attributes, void *priv),
+       void *priv)
 {
        const char *entry, *entry_end;
        char *name, *attributes;

@@ -93,7 +94,7 @@ int env_attr_walk(const char *attr_list,
                        if (strlen(name) != 0) {
                                int retval = 0;
 
-                               retval = callback(name, attributes);
+                               retval = callback(name, attributes, priv);
                                if (retval) {
                                        free(entry_cpy);
                                        return retval;

common/env_attr.c static int reverse_name_search(const char *searched, const char *search_for, · line 128

Quoted 10 lines

common/env_attr.c +5 -2

@@ -120,8 +121,11 @@ static int reverse_name_search(const char *searched, const char *search_for,
        if (result)
                *result = NULL;
 
-       if (*search_for == '\0')
-               return (char *)searched;
+       if (*search_for == '\0') {
+               if (result)
+                       *result = searched;
+               return strlen(searched);
+       }

I noticed shortly after sending this that this hunk belongs in the previous patch.

common/env_attr.c static int reverse_name_search(const char *searched, const char *search_for, · line 161

Quoted 10 lines

common/env_attr.c +2 -1

        for (;;) {
                const char *match = strstr(cur_searched, search_for);

@@ -153,7 +157,8 @@ static int reverse_name_search(const char *searched, const char *search_for,
                    *nextch != '\0')
                        continue;
 
-               *result = match;
+               if (result)
+                       *result = match;

As does this hunk.

common/env_attr.c static int reverse_name_search(const char *searched, const char *search_for, · line 164

Quoted 3 lines

common/env_attr.c +0 -0

                result_size = strlen(search_for);
        }
 

8<--snip-->8

My apologies... I'll resend after any other comments. Please do not apply as is, Tom.

Thanks,
-Joe

Allow the features that use env_attrs to specify regexs for the name

Signed-off-by Joe Hershberger <[email protected]>

---

 common/env_attr.c      | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/env_callback.h | 10 ++++--
 2 files changed, 93 insertions(+), 2 deletions(-)

We already could tell the difference in the callback between an import and "other" which we called interactive. Now add further distinction between interactive (i.e. running env set / env edit / env ask / etc. from the U-Boot command line) and programmatic (i.e. when u-boot source calls any variant of setenv() ).

Signed-off-by Joe Hershberger <[email protected]>

---

 common/cmd_nvedit.c | 26 +++++++++++++++++++-------
 include/search.h    |  2 ++
 2 files changed, 21 insertions(+), 7 deletions(-)

Use a regular expression to apply the default formatting flags for all ethaddr env vars.

Signed-off-by Joe Hershberger <[email protected]>

---

 include/env_flags.h | 11 ++++++++---
 test/dm/eth.c       |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

include/env_flags.h +8 -3

index 3ef6311..fc6d0d8 100644
@@ -38,13 +38,18 @@ enum env_flags_varaccess {
 #endif
 
 #ifdef CONFIG_CMD_NET
+#ifdef CONFIG_REGEX
+#define ETHADDR_WILDCARD "\\d?"
+#else
+#define ETHADDR_WILDCARD
+#endif
 #ifdef CONFIG_ENV_OVERWRITE
-#define ETHADDR_FLAGS "ethaddr:ma,"
+#define ETHADDR_FLAGS "eth" ETHADDR_WILDCARD "addr:ma,"
 #else
 #ifdef CONFIG_OVERWRITE_ETHADDR_ONCE
-#define ETHADDR_FLAGS "ethaddr:mc,"
+#define ETHADDR_FLAGS "eth" ETHADDR_WILDCARD "addr:mc,"
 #else
-#define ETHADDR_FLAGS "ethaddr:mo,"
+#define ETHADDR_FLAGS "eth" ETHADDR_WILDCARD "addr:mo,"
 #endif
 #endif
 #else

test/dm/eth.c +0 -0

index 4891f3a..9b714a1 100644

test/dm/eth.c +1 -1

+       setenv(".flags", "eth1addr");
        setenv("eth1addr", NULL);
 
        /* Make sure that the default is to rotate to the next interface */
--

1.7.11.5

Quoted 9 lines · Joe Hershberger

Use a regular expression to apply the default formatting flags for all ethaddr env vars.

Signed-off-by Joe Hershberger <[email protected]>

---

 include/env_flags.h | 11 ++++++++---
 test/dm/eth.c       |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

Reviewed-by Simon Glass <[email protected]>

Q below.

Quoted 29 lines

include/env_flags.h +8 -3

index 3ef6311..fc6d0d8 100644
@@ -38,13 +38,18 @@ enum env_flags_varaccess {
 #endif
 
 #ifdef CONFIG_CMD_NET
+#ifdef CONFIG_REGEX
+#define ETHADDR_WILDCARD "\\d?"
+#else
+#define ETHADDR_WILDCARD
+#endif
 #ifdef CONFIG_ENV_OVERWRITE
-#define ETHADDR_FLAGS "ethaddr:ma,"
+#define ETHADDR_FLAGS "eth" ETHADDR_WILDCARD "addr:ma,"
 #else
 #ifdef CONFIG_OVERWRITE_ETHADDR_ONCE
-#define ETHADDR_FLAGS "ethaddr:mc,"
+#define ETHADDR_FLAGS "eth" ETHADDR_WILDCARD "addr:mc,"
 #else
-#define ETHADDR_FLAGS "ethaddr:mo,"
+#define ETHADDR_FLAGS "eth" ETHADDR_WILDCARD "addr:mo,"
 #endif
 #endif
 #else

test/dm/eth.c +0 -0

index 4891f3a..9b714a1 100644

test/dm/eth.c static int dm_test_eth_rotate(struct dm_test_state *dms) · line 91

Quoted 6 lines

test/dm/eth.c +0 -0

@@ -89,6 +89,7 @@ static int dm_test_eth_rotate(struct dm_test_state *dms)
        /* Invalidate eth1's MAC address */
        net_ping_ip = string_to_ip("1.1.2.2");
        strcpy(ethaddr, getenv("eth1addr"));

Can you explain this next line, please?

Quoted 1 line

test/dm/eth.c +1 -0

+       setenv(".flags", "eth1addr");

Quoted 6 lines

test/dm/eth.c +0 -1

        setenv("eth1addr", NULL);
 
        /* Make sure that the default is to rotate to the next interface */
--

1.7.11.5

Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Hi Simon,

Quoted 44 lines · Simon Glass

Quoted 9 lines · Joe Hershberger

Use a regular expression to apply the default formatting flags for all ethaddr env vars.

Signed-off-by Joe Hershberger <[email protected]>

---

 include/env_flags.h | 11 ++++++++---
 test/dm/eth.c       |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

Reviewed-by Simon Glass <[email protected]>

Q below.

Quoted 29 lines

include/env_flags.h +8 -3

index 3ef6311..fc6d0d8 100644
@@ -38,13 +38,18 @@ enum env_flags_varaccess {
 #endif
 
 #ifdef CONFIG_CMD_NET
+#ifdef CONFIG_REGEX
+#define ETHADDR_WILDCARD "\\d?"
+#else
+#define ETHADDR_WILDCARD
+#endif
 #ifdef CONFIG_ENV_OVERWRITE
-#define ETHADDR_FLAGS "ethaddr:ma,"
+#define ETHADDR_FLAGS "eth" ETHADDR_WILDCARD "addr:ma,"
 #else
 #ifdef CONFIG_OVERWRITE_ETHADDR_ONCE
-#define ETHADDR_FLAGS "ethaddr:mc,"
+#define ETHADDR_FLAGS "eth" ETHADDR_WILDCARD "addr:mc,"
 #else
-#define ETHADDR_FLAGS "ethaddr:mo,"
+#define ETHADDR_FLAGS "eth" ETHADDR_WILDCARD "addr:mo,"
 #endif
 #endif
 #else

test/dm/eth.c +0 -0

index 4891f3a..9b714a1 100644

Quoted 10 lines

test/dm/eth.c static int dm_test_eth_rotate(struct dm_test_state *dms) · line 91

Quoted 6 lines

test/dm/eth.c +0 -0

@@ -89,6 +89,7 @@ static int dm_test_eth_rotate(struct dm_test_state *dms)
        /* Invalidate eth1's MAC address */
        net_ping_ip = string_to_ip("1.1.2.2");
        strcpy(ethaddr, getenv("eth1addr"));

Can you explain this next line, please?

Quoted 1 line

test/dm/eth.c +1 -0

+       setenv(".flags", "eth1addr");

This is now needed to allow the eth1addr to be modified. The env variable ".flags" overrides the static list that is compiled in. Since the regex now applies the ethaddr rules to eth\d?addr all of the ethaddr vars are restricted to write-once by default. You'll notice in another test where this was already overridden for "ethaddr" since it already had the flags applied.

This line has the effect of changing the flags for that variable to be "default" (i.e. unrestricted).

Quoted 6 lines

Quoted 6 lines

test/dm/eth.c +0 -1

        setenv("eth1addr", NULL);
 
        /* Make sure that the default is to rotate to the next interface */
--

1.7.11.5

Instead of checking for changes to the env each time we enter the net_loop, use the env callbacks to update the values of the variables. Don't update the variables when the source was programmatic, since the variables were the source of the new value.

Signed-off-by Joe Hershberger <[email protected]>

---

 include/env_callback.h |  22 ++++++++++-
 net/net.c              | 105 +++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 110 insertions(+), 17 deletions(-)

include/env_callback.h +21 -1

index 3de1093..91f3cc0 100644
@@ -37,6 +37,26 @@
 #define ENV_DOT_ESCAPE
 #endif
 
+#ifdef CONFIG_CMD_DNS
+#define DNS_CALLBACK "dnsip:dnsip,"
+#else
+#define DNS_CALLBACK
+#endif
+
+#ifdef CONFIG_NET
+#define NET_CALLBACKS \
+       "bootfile:bootfile," \
+       "ipaddr:ipaddr," \
+       "gatewayip:gatewayip," \
+       "netmask:netmask," \
+       "serverip:serverip," \
+       "nvlan:nvlan," \
+       "vlan:vlan," \
+       DNS_CALLBACK
+#else
+#define NET_CALLBACKS
+#endif
+
 /*
  * This list of callback bindings is static, but may be overridden by defining
  * a new association in the ".callbacks" environment variable.

@@ -44,7 +64,7 @@
 #define ENV_CALLBACK_LIST_STATIC ENV_DOT_ESCAPE ENV_CALLBACK_VAR ":callbacks," \
        ENV_DOT_ESCAPE ENV_FLAGS_VAR ":flags," \
        "baudrate:baudrate," \
-       "bootfile:bootfile," \
+       NET_CALLBACKS \
        "loadaddr:loadaddr," \
        SILENT_CALLBACK \
        SPLASHIMAGE_CALLBACK \

net/net.c +3 -0

index a365df0..57111ad 100644
@@ -208,6 +208,9 @@ 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_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
        switch (op) {
        case env_op_create:
        case env_op_overwrite:

net/net.c +82 -8

+               return 0;
+
+       net_ip = string_to_ip(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(ipaddr, on_ipaddr);
+
+static int on_gatewayip(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_gateway = string_to_ip(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(gatewayip, on_gatewayip);
+
+static int on_netmask(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_netmask = string_to_ip(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(netmask, on_netmask);
+
+static int on_serverip(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_server_ip = string_to_ip(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(serverip, on_serverip);
+
+static int on_nvlan(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_native_vlan = string_to_vlan(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(nvlan, on_nvlan);
+
+static int on_vlan(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_our_vlan = string_to_vlan(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(vlan, on_vlan);
+
+#if defined(CONFIG_CMD_DNS)
+static int on_dnsip(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_dns_server = string_to_ip(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(dnsip, on_dnsip);
+#endif
+
 /*
  * Check if autoload is enabled. If so, use either NFS or TFTP to download
  * the boot file.

@@ -252,22 +341,6 @@ void net_auto_load(void)
 
 static void net_init_loop(void)
 {
-       static int env_changed_id;
-       int env_id = get_env_id();
-
-       /* update only when the environment has changed */
-       if (env_changed_id != env_id) {
-               net_ip = getenv_ip("ipaddr");
-               net_gateway = getenv_ip("gatewayip");
-               net_netmask = getenv_ip("netmask");

Hi Joe,

Quoted 11 lines · Joe Hershberger

Instead of checking for changes to the env each time we enter the net_loop, use the env callbacks to update the values of the variables. Don't update the variables when the source was programmatic, since the variables were the source of the new value.

Signed-off-by Joe Hershberger <[email protected]>

---

 include/env_callback.h |  22 ++++++++++-
 net/net.c              | 105 +++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 110 insertions(+), 17 deletions(-)

Reviewed-by Simon Glass <[email protected]>

Q below.

Quoted 59 lines

include/env_callback.h +21 -1

index 3de1093..91f3cc0 100644
@@ -37,6 +37,26 @@
 #define ENV_DOT_ESCAPE
 #endif
 
+#ifdef CONFIG_CMD_DNS
+#define DNS_CALLBACK "dnsip:dnsip,"
+#else
+#define DNS_CALLBACK
+#endif
+
+#ifdef CONFIG_NET
+#define NET_CALLBACKS \
+       "bootfile:bootfile," \
+       "ipaddr:ipaddr," \
+       "gatewayip:gatewayip," \
+       "netmask:netmask," \
+       "serverip:serverip," \
+       "nvlan:nvlan," \
+       "vlan:vlan," \
+       DNS_CALLBACK
+#else
+#define NET_CALLBACKS
+#endif
+
 /*
  * This list of callback bindings is static, but may be overridden by defining
  * a new association in the ".callbacks" environment variable.

@@ -44,7 +64,7 @@
 #define ENV_CALLBACK_LIST_STATIC ENV_DOT_ESCAPE ENV_CALLBACK_VAR ":callbacks," \
        ENV_DOT_ESCAPE ENV_FLAGS_VAR ":flags," \
        "baudrate:baudrate," \
-       "bootfile:bootfile," \
+       NET_CALLBACKS \
        "loadaddr:loadaddr," \
        SILENT_CALLBACK \
        SPLASHIMAGE_CALLBACK \

net/net.c +3 -0

index a365df0..57111ad 100644
@@ -208,6 +208,9 @@ 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_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
        switch (op) {
        case env_op_create:
        case env_op_overwrite:

@@ -222,6 +225,92 @@ static int on_bootfile(const char *name, const char *value, enum env_op op,
 }
 U_BOOT_ENV_CALLBACK(bootfile, on_bootfile);
 

net/net.c static int on_bootfile(const char *name, const char *value, enum env_op op, · line 231

Quoted 4 lines

net/net.c +4 -0

+static int on_ipaddr(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)

Can you just do this?

if (flags & H_PROGRAMMATIC)

Quoted 97 lines

net/net.c +82 -8

+               return 0;
+
+       net_ip = string_to_ip(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(ipaddr, on_ipaddr);
+
+static int on_gatewayip(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_gateway = string_to_ip(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(gatewayip, on_gatewayip);
+
+static int on_netmask(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_netmask = string_to_ip(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(netmask, on_netmask);
+
+static int on_serverip(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_server_ip = string_to_ip(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(serverip, on_serverip);
+
+static int on_nvlan(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_native_vlan = string_to_vlan(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(nvlan, on_nvlan);
+
+static int on_vlan(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_our_vlan = string_to_vlan(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(vlan, on_vlan);
+
+#if defined(CONFIG_CMD_DNS)
+static int on_dnsip(const char *name, const char *value, enum env_op op,
+       int flags)
+{
+       if ((flags & H_ORIGIN_FLAGS) == H_PROGRAMMATIC)
+               return 0;
+
+       net_dns_server = string_to_ip(value);
+
+       return 0;
+}
+U_BOOT_ENV_CALLBACK(dnsip, on_dnsip);
+#endif
+
 /*
  * Check if autoload is enabled. If so, use either NFS or TFTP to download
  * the boot file.

@@ -252,22 +341,6 @@ void net_auto_load(void)
 
 static void net_init_loop(void)
 {
-       static int env_changed_id;
-       int env_id = get_env_id();
-
-       /* update only when the environment has changed */
-       if (env_changed_id != env_id) {
-               net_ip = getenv_ip("ipaddr");
-               net_gateway = getenv_ip("gatewayip");
-               net_netmask = getenv_ip("netmask");

net/net.c void net_auto_load(void) · line 345

Quoted 10 lines

net/net.c +0 -8

-               net_server_ip = getenv_ip("serverip");
-               net_native_vlan = getenv_vlan("nvlan");
-               net_our_vlan = getenv_vlan("vlan");
-#if defined(CONFIG_CMD_DNS)
-               net_dns_server = getenv_ip("dnsip");
-#endif
-               env_changed_id = env_id;
-       }
        if (eth_get_dev())
                memcpy(net_ethaddr, eth_get_ethaddr(), 6);

Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Check that the common network stack's env vars conform to the proper format for IP addresses.

Signed-off-by Joe Hershberger <[email protected]>

---

 include/env_flags.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

This includes moving CONFIG_REGEX to Kconfig and adding support for regex to the env_attr lists (when CONFIG_REGEX is enabled).

This allows ethaddrs to all be checked for access and format by default. Also use callbacks to keep network stack variables up to date instead of polling them on each call to net_loop.

This is a step in the right direction to refactoring the network stack to be similar to that of barebox.

Joe Hershberger (11):
sandbox: Enable some ENV commands
kconfig: Move REGEX to Kconfig
sandbox: Enable regex support
env: Fix return values in env_attr_lookup()
env: Simplify the reverse_strstr() interface
env: Allow env_attr_walk to pass a priv * to callback
env: Add regex support to env_attrs
env: Distinguish finer between source of env change
net: Apply default format rules to all ethaddr
net: Use env callbacks for net variables
net: Add default flags for common net env vars

 common/cmd_nvedit.c                |  36 +++++---
 common/env_attr.c                  | 179 ++++++++++++++++++++++++++++---------
 common/env_callback.c              |   6 +-
 common/env_flags.c                 |   6 +-
 configs/acadia_defconfig           |   1 +
 configs/bamboo_defconfig           |   1 +
 configs/bubinga_defconfig          |   1 +
 configs/canyonlands_defconfig      |   1 +
 configs/dlvision-10g_defconfig     |   1 +
 configs/dlvision_defconfig         |   1 +
 configs/ebony_defconfig            |   1 +
 configs/gdppc440etx_defconfig      |   1 +
 configs/icon_defconfig             |   1 +
 configs/intip_defconfig            |   1 +
 configs/io64_defconfig             |   1 +
 configs/io_defconfig               |   1 +
 configs/iocon_defconfig            |   1 +
 configs/katmai_defconfig           |   1 +
 configs/kilauea_defconfig          |   1 +
 configs/luan_defconfig             |   1 +
 configs/m28evk_defconfig           |   1 +
 configs/m53evk_defconfig           |   1 +
 configs/makalu_defconfig           |   1 +
 configs/neo_defconfig              |   1 +
 configs/novena_defconfig           |   1 +
 configs/ocotea_defconfig           |   1 +
 configs/redwood_defconfig          |   1 +
 configs/sandbox_defconfig          |   1 +
 configs/sequoia_defconfig          |   1 +
 configs/socfpga_arria5_defconfig   |   1 +
 configs/socfpga_cyclone5_defconfig |   1 +
 configs/t3corp_defconfig           |   1 +
 configs/taihu_defconfig            |   1 +
 configs/taishan_defconfig          |   1 +
 configs/walnut_defconfig           |   1 +
 configs/yosemite_defconfig         |   1 +
 configs/yucca_defconfig            |   1 +
 include/configs/amcc-common.h      |   1 -
 include/configs/m28evk.h           |   1 -
 include/configs/m53evk.h           |   1 -
 include/configs/novena.h           |   1 -
 include/configs/sandbox.h          |   5 ++
 include/configs/socfpga_arria5.h   |   1 -
 include/configs/socfpga_cyclone5.h |   1 -

 include/env_attr.h                 |  10 +--
 include/env_callback.h             |  32 ++++++-
 include/env_flags.h                |  23 ++++-
 include/search.h                   |   2 +
 lib/Kconfig                        |   8 ++
 net/net.c                          | 105 ++++++++++++++++++----
 test/dm/eth.c                      |   1 +
 51 files changed, 358 insertions(+), 94 deletions(-)

Hi Joe,

Quoted 68 lines · Joe Hershberger

This includes moving CONFIG_REGEX to Kconfig and adding support for regex to the env_attr lists (when CONFIG_REGEX is enabled).

This allows ethaddrs to all be checked for access and format by default. Also use callbacks to keep network stack variables up to date instead of polling them on each call to net_loop.

This is a step in the right direction to refactoring the network stack to be similar to that of barebox.

Joe Hershberger (11):
sandbox: Enable some ENV commands
kconfig: Move REGEX to Kconfig
sandbox: Enable regex support
env: Fix return values in env_attr_lookup()
env: Simplify the reverse_strstr() interface
env: Allow env_attr_walk to pass a priv * to callback
env: Add regex support to env_attrs
env: Distinguish finer between source of env change
net: Apply default format rules to all ethaddr
net: Use env callbacks for net variables
net: Add default flags for common net env vars

 common/cmd_nvedit.c                |  36 +++++---
 common/env_attr.c                  | 179 ++++++++++++++++++++++++++++---------
 common/env_callback.c              |   6 +-
 common/env_flags.c                 |   6 +-
 configs/acadia_defconfig           |   1 +
 configs/bamboo_defconfig           |   1 +
 configs/bubinga_defconfig          |   1 +
 configs/canyonlands_defconfig      |   1 +
 configs/dlvision-10g_defconfig     |   1 +
 configs/dlvision_defconfig         |   1 +
 configs/ebony_defconfig            |   1 +
 configs/gdppc440etx_defconfig      |   1 +
 configs/icon_defconfig             |   1 +
 configs/intip_defconfig            |   1 +
 configs/io64_defconfig             |   1 +
 configs/io_defconfig               |   1 +
 configs/iocon_defconfig            |   1 +
 configs/katmai_defconfig           |   1 +
 configs/kilauea_defconfig          |   1 +
 configs/luan_defconfig             |   1 +
 configs/m28evk_defconfig           |   1 +
 configs/m53evk_defconfig           |   1 +
 configs/makalu_defconfig           |   1 +
 configs/neo_defconfig              |   1 +
 configs/novena_defconfig           |   1 +
 configs/ocotea_defconfig           |   1 +
 configs/redwood_defconfig          |   1 +
 configs/sandbox_defconfig          |   1 +
 configs/sequoia_defconfig          |   1 +
 configs/socfpga_arria5_defconfig   |   1 +
 configs/socfpga_cyclone5_defconfig |   1 +
 configs/t3corp_defconfig           |   1 +
 configs/taihu_defconfig            |   1 +
 configs/taishan_defconfig          |   1 +
 configs/walnut_defconfig           |   1 +
 configs/yosemite_defconfig         |   1 +
 configs/yucca_defconfig            |   1 +
 include/configs/amcc-common.h      |   1 -
 include/configs/m28evk.h           |   1 -
 include/configs/m53evk.h           |   1 -
 include/configs/novena.h           |   1 -
 include/configs/sandbox.h          |   5 ++
 include/configs/socfpga_arria5.h   |   1 -
 include/configs/socfpga_cyclone5.h |   1 -

Quoted 8 lines

 include/env_attr.h                 |  10 +--
 include/env_callback.h             |  32 ++++++-
 include/env_flags.h                |  23 ++++-
 include/search.h                   |   2 +
 lib/Kconfig                        |   8 ++
 net/net.c                          | 105 ++++++++++++++++++----
 test/dm/eth.c                      |   1 +
 51 files changed, 358 insertions(+), 94 deletions(-)

Looks good! I wonder if you could update a README somewhere to explain how it works?

If I understand correctly, you need to enable CONFIG_REGEX for the eth1addr variable to work (for example). Is that right? If so, what is the code size impact?

Hi Simon,

Quoted 80 lines · Simon Glass

Hi Joe,

Quoted 68 lines · Joe Hershberger

This includes moving CONFIG_REGEX to Kconfig and adding support for regex to the env_attr lists (when CONFIG_REGEX is enabled).

This allows ethaddrs to all be checked for access and format by default. Also use callbacks to keep network stack variables up to date instead of polling them on each call to net_loop.

This is a step in the right direction to refactoring the network stack to be similar to that of barebox.

Joe Hershberger (11):
sandbox: Enable some ENV commands
kconfig: Move REGEX to Kconfig
sandbox: Enable regex support
env: Fix return values in env_attr_lookup()
env: Simplify the reverse_strstr() interface
env: Allow env_attr_walk to pass a priv * to callback
env: Add regex support to env_attrs
env: Distinguish finer between source of env change
net: Apply default format rules to all ethaddr
net: Use env callbacks for net variables
net: Add default flags for common net env vars

 common/cmd_nvedit.c                |  36 +++++---
 common/env_attr.c                  | 179 ++++++++++++++++++++++++++++---------
 common/env_callback.c              |   6 +-
 common/env_flags.c                 |   6 +-
 configs/acadia_defconfig           |   1 +
 configs/bamboo_defconfig           |   1 +
 configs/bubinga_defconfig          |   1 +
 configs/canyonlands_defconfig      |   1 +
 configs/dlvision-10g_defconfig     |   1 +
 configs/dlvision_defconfig         |   1 +
 configs/ebony_defconfig            |   1 +
 configs/gdppc440etx_defconfig      |   1 +
 configs/icon_defconfig             |   1 +
 configs/intip_defconfig            |   1 +
 configs/io64_defconfig             |   1 +
 configs/io_defconfig               |   1 +
 configs/iocon_defconfig            |   1 +
 configs/katmai_defconfig           |   1 +
 configs/kilauea_defconfig          |   1 +
 configs/luan_defconfig             |   1 +
 configs/m28evk_defconfig           |   1 +
 configs/m53evk_defconfig           |   1 +
 configs/makalu_defconfig           |   1 +
 configs/neo_defconfig              |   1 +
 configs/novena_defconfig           |   1 +
 configs/ocotea_defconfig           |   1 +
 configs/redwood_defconfig          |   1 +
 configs/sandbox_defconfig          |   1 +
 configs/sequoia_defconfig          |   1 +
 configs/socfpga_arria5_defconfig   |   1 +
 configs/socfpga_cyclone5_defconfig |   1 +
 configs/t3corp_defconfig           |   1 +
 configs/taihu_defconfig            |   1 +
 configs/taishan_defconfig          |   1 +
 configs/walnut_defconfig           |   1 +
 configs/yosemite_defconfig         |   1 +
 configs/yucca_defconfig            |   1 +
 include/configs/amcc-common.h      |   1 -
 include/configs/m28evk.h           |   1 -
 include/configs/m53evk.h           |   1 -
 include/configs/novena.h           |   1 -
 include/configs/sandbox.h          |   5 ++
 include/configs/socfpga_arria5.h   |   1 -
 include/configs/socfpga_cyclone5.h |   1 -

Quoted 8 lines

 include/env_attr.h                 |  10 +--
 include/env_callback.h             |  32 ++++++-
 include/env_flags.h                |  23 ++++-
 include/search.h                   |   2 +
 lib/Kconfig                        |   8 ++
 net/net.c                          | 105 ++++++++++++++++++----
 test/dm/eth.c                      |   1 +
 51 files changed, 358 insertions(+), 94 deletions(-)

Quoted 2 lines

Looks good! I wonder if you could update a README somewhere to explain how it works?

I'll update README to describe it.

Quoted 3 lines

If I understand correctly, you need to enable CONFIG_REGEX for the eth1addr variable to work (for example). Is that right? If so, what is the code size impact?

That's sort-of correct. Before the regex, only the "ethaddr" was checked for format, though the eth1addr, etc. all "worked", just were unverified.

I did some build tests...

Without CONFIG_REGEX, the total size grew by 295 bytes (due to the other global changes).
With CONFIG_REGEX enabled, the total size grew by 378 bytes. Enabling CONFIG_REGEX now adds 3633 bytes total.

This test was on BB Black (ARM).

Raw data:

text data bss dec hex filename
W/ patch W/O regex
394014 13324 305876 713214 ae1fe /tmp/u-boot-build/arm/u-boot
W/ patch W/ regex
397651 13324 305872 716847 af02f /tmp/u-boot-build/arm/u-boot
W/O patch W/O regex
393811 13276 305832 712919 ae0d7 /tmp/u-boot-build/arm/u-boot
W/O patch W/ regex
397333 13276 305860 716469 aeeb5 /tmp/u-boot-build/arm/u-boot

Cheers,
-Joe

Hi Joe,

Quoted 96 lines · Joe Hershberger

Hi Simon,

Quoted 80 lines · Simon Glass

Hi Joe,

Quoted 68 lines · Joe Hershberger

This includes moving CONFIG_REGEX to Kconfig and adding support for regex to the env_attr lists (when CONFIG_REGEX is enabled).

This allows ethaddrs to all be checked for access and format by default. Also use callbacks to keep network stack variables up to date instead of polling them on each call to net_loop.

This is a step in the right direction to refactoring the network stack to be similar to that of barebox.

Joe Hershberger (11):
sandbox: Enable some ENV commands
kconfig: Move REGEX to Kconfig
sandbox: Enable regex support
env: Fix return values in env_attr_lookup()
env: Simplify the reverse_strstr() interface
env: Allow env_attr_walk to pass a priv * to callback
env: Add regex support to env_attrs
env: Distinguish finer between source of env change
net: Apply default format rules to all ethaddr
net: Use env callbacks for net variables
net: Add default flags for common net env vars

 common/cmd_nvedit.c                |  36 +++++---
 common/env_attr.c                  | 179 ++++++++++++++++++++++++++++---------
 common/env_callback.c              |   6 +-
 common/env_flags.c                 |   6 +-
 configs/acadia_defconfig           |   1 +
 configs/bamboo_defconfig           |   1 +
 configs/bubinga_defconfig          |   1 +
 configs/canyonlands_defconfig      |   1 +
 configs/dlvision-10g_defconfig     |   1 +
 configs/dlvision_defconfig         |   1 +
 configs/ebony_defconfig            |   1 +
 configs/gdppc440etx_defconfig      |   1 +
 configs/icon_defconfig             |   1 +
 configs/intip_defconfig            |   1 +
 configs/io64_defconfig             |   1 +
 configs/io_defconfig               |   1 +
 configs/iocon_defconfig            |   1 +
 configs/katmai_defconfig           |   1 +
 configs/kilauea_defconfig          |   1 +
 configs/luan_defconfig             |   1 +
 configs/m28evk_defconfig           |   1 +
 configs/m53evk_defconfig           |   1 +
 configs/makalu_defconfig           |   1 +
 configs/neo_defconfig              |   1 +
 configs/novena_defconfig           |   1 +
 configs/ocotea_defconfig           |   1 +
 configs/redwood_defconfig          |   1 +
 configs/sandbox_defconfig          |   1 +
 configs/sequoia_defconfig          |   1 +
 configs/socfpga_arria5_defconfig   |   1 +
 configs/socfpga_cyclone5_defconfig |   1 +
 configs/t3corp_defconfig           |   1 +
 configs/taihu_defconfig            |   1 +
 configs/taishan_defconfig          |   1 +
 configs/walnut_defconfig           |   1 +
 configs/yosemite_defconfig         |   1 +
 configs/yucca_defconfig            |   1 +
 include/configs/amcc-common.h      |   1 -
 include/configs/m28evk.h           |   1 -
 include/configs/m53evk.h           |   1 -
 include/configs/novena.h           |   1 -
 include/configs/sandbox.h          |   5 ++
 include/configs/socfpga_arria5.h   |   1 -
 include/configs/socfpga_cyclone5.h |   1 -

Quoted 8 lines

 include/env_attr.h                 |  10 +--
 include/env_callback.h             |  32 ++++++-
 include/env_flags.h                |  23 ++++-
 include/search.h                   |   2 +
 lib/Kconfig                        |   8 ++
 net/net.c                          | 105 ++++++++++++++++++----
 test/dm/eth.c                      |   1 +
 51 files changed, 358 insertions(+), 94 deletions(-)

Quoted 2 lines

Looks good! I wonder if you could update a README somewhere to explain how it works?

I'll update README to describe it.

Quoted 3 lines

If I understand correctly, you need to enable CONFIG_REGEX for the eth1addr variable to work (for example). Is that right? If so, what is the code size impact?

That's sort-of correct. Before the regex, only the "ethaddr" was checked for format, though the eth1addr, etc. all "worked", just were unverified.

Quoted 6 lines

I did some build tests...

Without CONFIG_REGEX, the total size grew by 295 bytes (due to the other global changes).
With CONFIG_REGEX enabled, the total size grew by 378 bytes. Enabling CONFIG_REGEX now adds 3633 bytes total.

Probably because it is now actually being used?

This is definitely a size increase but IMO it is worth it and those using networking are likely less size-sensitive.

Quoted 15 lines

This test was on BB Black (ARM).

Raw data:

text data bss dec hex filename
W/ patch W/O regex
394014 13324 305876 713214 ae1fe /tmp/u-boot-build/arm/u-boot
W/ patch W/ regex
397651 13324 305872 716847 af02f /tmp/u-boot-build/arm/u-boot
W/O patch W/O regex
393811 13276 305832 712919 ae0d7 /tmp/u-boot-build/arm/u-boot
W/O patch W/ regex
397333 13276 305860 716469 aeeb5 /tmp/u-boot-build/arm/u-boot

Quoted 2 lines

Cheers,
-Joe

Regards,
Simon

Having this as a Kconfig allows it to be a dependent feature.

Signed-off-by Joe Hershberger <[email protected]>

Reviewed-by Simon Glass <[email protected]>