freecwmp
[Top] [All Lists]

Re: [PATCH 15/27] add the common function and adapt the script freecwmp.

To: mohamed.kallel@pivasoftware.com
Subject: Re: [PATCH 15/27] add the common function and adapt the script freecwmp.sh in order to support get parameter names Contributed by Inteno Broadband Technology AB
From: Luka Perkov <freecwmp@lukaperkov.net>
Date: Sat, 8 Dec 2012 11:49:40 +0100
Cc: freecwmp@linux-mips.org, ahmed.zribi@pivasoftware.com, freecwmp@lukaperkov.net, jogo@openwrt.org
In-reply-to: <1354809292-2467-16-git-send-email-mohamed.kallel@pivasoftware.com>
Mail-followup-to: mohamed.kallel@pivasoftware.com, freecwmp@linux-mips.org, ahmed.zribi@pivasoftware.com, jogo@openwrt.org
Original-recipient: rfc822;freecwmp@linux-mips.org
References: <mohamed.kallel@pivasoftware.com> <1354809292-2467-1-git-send-email-mohamed.kallel@pivasoftware.com> <1354809292-2467-16-git-send-email-mohamed.kallel@pivasoftware.com>
Sender: freecwmp-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
Missing commit message. Contributed by must go in commit message and not
in subject.

On Thu, Dec 06, 2012 at 04:54:40PM +0100, Mohamed wrote:
> 
> Signed-off-by: Mohamed <mohamed.kallel@pivasoftware.com>
> Signed-off-by: Ahmed ZRIBI <ahmed.zribi@pivasoftware.com>

We are using tabs and not spaces. This must be changed in the entire patch.

Also, I don't see that you have updated help output.

> ---
>  ext/openwrt/config/freecwmp          |    6 ++++
>  ext/openwrt/scripts/freecwmp.sh      |   47 
> ++++++++++++++++++++++++++++++++++
>  ext/openwrt/scripts/functions/common |    4 +++
>  3 files changed, 57 insertions(+), 0 deletions(-)
> 
> diff --git a/ext/openwrt/config/freecwmp b/ext/openwrt/config/freecwmp
> index ba6fbda..3b2b4c3 100644
> --- a/ext/openwrt/config/freecwmp
> +++ b/ext/openwrt/config/freecwmp
> @@ -29,19 +29,25 @@ config scripts
>       # freecwmp specific functions
>       list location /usr/share/freecwmp/functions/device_info
>       list get_value_function get_device_info
> +    list get_name_function get_device_info_name
>       list set_value_function set_device_info
>       list get_value_function get_device_info_generic
> +    list get_name_function get_device_info_generic_name
>       list set_value_function set_device_info_generic
>       list location /usr/share/freecwmp/functions/lan_device
>       list get_value_function get_lan_device
> +    list get_name_function get_lan_device_name
>       list set_value_function set_lan_device
>       list location /usr/share/freecwmp/functions/management_server
>       list get_value_function get_management_server
> +    list get_name_function get_management_server_name
>       list set_value_function set_management_server
>       list get_value_function get_management_server_generic
> +    list get_name_function get_management_server_generic_name
>       list set_value_function set_management_server_generic
>       list location /usr/share/freecwmp/functions/wan_device
>       list get_value_function get_wan_device
> +    list get_name_function get_wan_device_name
>       list set_value_function set_wan_device
>       list location /usr/share/freecwmp/functions/misc
>       list get_value_function get_misc
> diff --git a/ext/openwrt/scripts/freecwmp.sh b/ext/openwrt/scripts/freecwmp.sh
> index f2bebc8..c5a0d53 100644
> --- a/ext/openwrt/scripts/freecwmp.sh
> +++ b/ext/openwrt/scripts/freecwmp.sh
> @@ -68,6 +68,10 @@ case "$1" in
>               elif [ "$2" = "value" ]; then
>                       __arg1="$3"
>                       action="get_value"
> +        elif [ "$2" = "name" ]; then
> +            __arg1="$3"
> +            __arg2=`echo $4| tr '[A-Z]' '[a-z]'`

Please explain what are you trying to do here. Why do we need __arg2?

> +            action="get_name"
>               elif [ "$2" = "all" ]; then
>                       __arg1="$3"
>                       action="get_all"
> @@ -107,6 +111,7 @@ handle_scripts() {
>       config_get prefix "$section" "prefix"
>       config_list_foreach "$section" 'location' load_script
>       config_get get_value_functions "$section" "get_value_function"
> +    config_get get_name_functions "$section" "get_name_function"
>       config_get set_value_functions "$section" "set_value_function"
>  }
>  
> @@ -163,6 +168,48 @@ if [ "$action" = "get_value" -o "$action" = "get_all" ]; 
> then
>      fi
>  fi
>  
> +if [ "$action" = "get_name" -o "$action" = "get_all" ]; then
> +    no_fault="0"
> +    freecwmp_check_fault "$__arg1"
> +    fault_code="$?"
> +    if [ "$fault_code" = "0" ]; then
> +        if [ \( "$__arg2" != "0" \) -a \( "$__arg2" != "1" \) -a \( 
> "$__arg2" != "true" \) -a \( "$__arg2" != "false" \) ]; then
> +            fault_code="$FAULT_CPE_INVALID_ARGUMENTS"
> +        else
> +            if [ "$__arg2" = "true" ]; then
> +                __arg2="1"
> +            elif [ "$__arg2" = "false" ]; then
> +                __arg2="0"
> +            fi

I don't like that you are changing here __arxX variables. This needs to be
reworked.

> +        fi
> +        if [ "$fault_code" = "0" ]; then
> +            if [ \( "$__arg1" = "InternetGatewayDevice." \) -o \( "$__arg1" 
> = "" \) ]; then

Do we really need "\(" ?

> +                ubus_freecwmp_output "InternetGatewayDevice." "0"
> +                if [ \( "$__arg1" = "" \) -a \( "$__arg2" = "1" \) ]; then
> +                    exit 0
> +                fi
> +                __arg1="InternetGatewayDevice."
> +            fi
> +            for function_name in $get_name_functions
> +            do
> +                $function_name "$__arg1" "$__arg2"
> +                fault_code="$?"
> +                if [ "$fault_code" = "0" ]; then
> +                    no_fault="1"
> +                fi
> +                if [ "$fault_code" = "$FAULT_CPE_INVALID_ARGUMENTS" ]; then
> +                    break
> +                fi
> +     done
> +            if [ "$no_fault" = "1" ]; then fault_code="0"; fi

Use multiple lines here. Also, no_fault error checking can be optimized here.

> +        fi
> +    fi
> +    if [ "$fault_code" != "0" ]; then
> +        let fault_code=$fault_code+9000

Please explain the logic behind this. We should use 9000 when defining the
variables.

> +        ubus_freecwmp_output "$__arg1" "" "" "$fault_code"
> +    fi
> +fi
> +
>  if [ "$action" = "set_value" ]; then
>       for function_name in $set_value_functions
>       do
> diff --git a/ext/openwrt/scripts/functions/common 
> b/ext/openwrt/scripts/functions/common
> index 082e067..abba220 100644
> --- a/ext/openwrt/scripts/functions/common
> +++ b/ext/openwrt/scripts/functions/common
> @@ -48,6 +48,10 @@ case "$action" in
>       get_value)
>       ubus call tr069 get_parameter_values '{"parameter": "'$parameter'", 
> "value": "'$value'", "type": "'$type'", "fault_code":"'$fault_code'"}' 2> 
> /dev/null
>       ;;
> +     get_name)
> +     ubus call tr069 get_parameter_names '{"parameter": "'$parameter'", 
> "writable": "'$value'", "fault_code":"'$fault_code'"}' 2> /dev/null
> +     ;;

It's not good idea to use the same $value variable for two use cases.
This needs to be changed. Look at my comments in the next patch.

> + 

Remove this newline.

>  esac
>  }
>  freecwmp_value_output() {
> -- 
> 1.7.4.1

Luka 

<Prev in Thread] Current Thread [Next in Thread>