openwrt / packages

Community maintained packages for OpenWrt. Documentation for submitting pull requests is in CONTRIBUTING.md
GNU General Public License v2.0
4.01k stars 3.49k forks source link

net/bonding: proto handler not supporting list elements in config #11455

Closed ghost closed 3 years ago

ghost commented 4 years ago

Maintainer: ma@dev.tdt.de Environment: owrt-19.07, master

refs #6274

In former versions MultiValues and DynamicLists have been written as "option xxxx 'val1 val2 ...' by LuCI. Newer versions of LuCI (javascript) write them as "list xxxx 'val1'", "list xxxx 'val2'", ...

Now doing a 'proto_config_add_string "xxxx"'' in proto handler doesn't work, neither a 'proto_config_add_array "xxxx"'.

For now I'm just doing a workaround with explicitly loading the config (config_load) and then getting the values manually (config_get).

Would it be possible to implement a function for loading list values from config?

Regards

@jow- Any suggestions for this issue? Thanks in advance

qiuzi commented 3 years ago

@he-ma @jow- This problem is exactly what I said, how did you solve it in the end? bonding.sh script

jow- commented 3 years ago

@he-ma - proto_config_add_array() works just fine here. For testing, I locally modified my 6in4.sh like this:

diff --git a/package/network/ipv6/6in4/files/6in4.sh b/package/network/ipv6/6in4/files/6in4.sh
index 5b5c7b36af..9e354ad7ed 100755
--- a/package/network/ipv6/6in4/files/6in4.sh
+++ b/package/network/ipv6/6in4/files/6in4.sh
@@ -40,6 +40,10 @@ proto_6in4_add_prefix() {
        append "$3" "$1"
 }

+proto_6in4_dump_foo() {
+       echo "FOO-ITEM: $1"
+}
+
 proto_6in4_setup() {
        local cfg="$1"
        local iface="$2"
@@ -49,6 +53,12 @@ proto_6in4_setup() {
        json_get_vars mtu ttl tos ipaddr peeraddr ip6addr tunlink tunnelid username password updatekey
        json_for_each_item proto_6in4_add_prefix ip6prefix ip6prefixes

+       local foo
+       json_get_vars foo
+
+       echo "FOO=$foo"
+       json_for_each_item proto_6in4_dump_foo foo
+
        [ -z "$peeraddr" ] && {
                proto_notify_error "$cfg" "MISSING_ADDRESS"
                proto_block_restart "$cfg"
@@ -156,6 +166,7 @@ proto_6in4_init_config() {
        proto_config_add_int "mtu"
        proto_config_add_int "ttl"
        proto_config_add_string "tos"
+       proto_config_add_array foo
 }

 [ -n "$INCLUDE_ONLY" ] || {

Then I tried these uci snippets:

1. White space separated list
config interface 'wan6'
    option proto '6in4'
    ...
    option foo '1 2 3'
root@jj:~# ifup wan6; sleep 1; logread | grep FOO | tail -n 4
Fri Apr  9 14:53:05 2021 daemon.notice netifd: wan6 (17969): FOO=J_A2
Fri Apr  9 14:53:05 2021 daemon.notice netifd: wan6 (17969): FOO-ITEM: 1
Fri Apr  9 14:53:05 2021 daemon.notice netifd: wan6 (17969): FOO-ITEM: 2
Fri Apr  9 14:53:05 2021 daemon.notice netifd: wan6 (17969): FOO-ITEM: 3
2. List notation
config interface 'wan6'
    option proto '6in4'
    ...
    list foo '4'
        list foo '5'
        list foo '6'
root@jj:~# ifup wan6; sleep 1; logread | grep FOO | tail -n 4
Fri Apr  9 14:54:33 2021 daemon.notice netifd: wan6 (18495): FOO=J_A2
Fri Apr  9 14:54:33 2021 daemon.notice netifd: wan6 (18495): FOO-ITEM: 4
Fri Apr  9 14:54:33 2021 daemon.notice netifd: wan6 (18495): FOO-ITEM: 5
Fri Apr  9 14:54:33 2021 daemon.notice netifd: wan6 (18495): FOO-ITEM: 6

So maybe your mistake was to not iterate using json_for_each_item() when you attempted to switch to proto_config_add_array() ?

ghost commented 3 years ago

@qiuzi Ah, now I think I understand - maybe my bad. As jow just replied to my issue (https://github.com/openwrt/packages/issues/11455) I now remember. I made a local workaround in bonding.sh around one year ago which I use.

I'm sorry I didn't understand your problem because my workaround and that I could not reproduce it. As I have the modifications only locally, it is nowhere comitted.

Would my (unofficial) modified script help you?

ghost commented 3 years ago

@jow- Thank you for your clarification and explanation. I will modify my code in the near future (as I'm a bit busy at the moment).