freecwmp
[Top] [All Lists]

Re: [PATCH] freecwmp: make cwmp version configurable

To: Kaspar Schleiser <kaspar@schleiser.de>
Subject: Re: [PATCH] freecwmp: make cwmp version configurable
From: Luka Perkov <freecwmp@lukaperkov.net>
Date: Wed, 22 Feb 2012 16:23:29 +0100
Authentication-results: mr.google.com; spf=pass (google.com: domain of freecwmp@lukaperkov.net designates 10.213.28.136 as permitted sender) smtp.mail=freecwmp@lukaperkov.net
Cc: freecwmp@linux-mips.org
In-reply-to: <1329917025-15282-1-git-send-email-kaspar@schleiser.de>
Mail-followup-to: Kaspar Schleiser <kaspar@schleiser.de>, freecwmp@linux-mips.org
Original-recipient: rfc822;freecwmp@linux-mips.org
References: <1329917025-15282-1-git-send-email-kaspar@schleiser.de>
Sender: freecwmp-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
Hi Kaspar,

first I would like to say thank you for the patch. I would like to point
out few things:

 * use tabs instead of spaces
 * avoid trailing wihite spaces

On Wed, Feb 22, 2012 at 02:23:45PM +0100, Kaspar Schleiser wrote:
> currently, freecwmp hard-codes cwmp protocol version 1.2.
> 
> here's a patch that adds a configuration option for the server's cwmp version.
> It also changes cwmp-urn and cwmp:ID handling according to the version 
> specified by the user.
> 
> Currently "1.0" and "1.2" are being recognized.
> 
> Default is 1.2, so for upgraders nothing changes, missing option is no 
> problem. 
> Users of 1.0 servers can set "cwmp_version" to "1.0" and start rocking 
> oldschool.

We should use 1.0 as default one because it's the one most commonly
used.

I looked at the documentation:

http://www.broadband-forum.org/cwmp.php
http://www.broadband-forum.org/cwmp/cwmp-1-0.xsd
http://www.broadband-forum.org/cwmp/cwmp-1-1.xsd
http://www.broadband-forum.org/cwmp/cwmp-1-2.xsd
http://www.broadband-forum.org/cwmp/cwmp-1-3.xsd

And there are versions 1.0, 1.1, 1.2 and surprisingly no 1.3 version.
I'm thinking why not pass the config string to the xml.c? That way user
can write say 1.9 or whatever and it will get passed to the ACS. By the
default we will put "1.0" and that should not be a issue then. If the
user abuses this communication with ACS might not work, but it will be
his problem then. What do you think about this?

Also, I would rename "cwmp_version" to only "version".

I'm still not well and I can not do much work. If you do not resend the
patch I'll rework this one and merge it.

See rest of my comments inline.

Luka


> diff --git a/src/config.c b/src/config.c
> index ea73c51..20aa685 100644
> --- a/src/config.c
> +++ b/src/config.c
> @@ -175,6 +175,24 @@ section_found:
>                       n++;
>                       goto next;
>               }
> +             

example of trailing whitespace

> +        /* cwmp version */

example of spaces instead of tabs

> +             status = strcmp((uci_to_option(e))->e.name, "cwmp_version");
> +             if (status == FC_SUCCESS) {
> +            char* c = uci_to_option(e)->v.string;
> +            enum cwmp_version version = V1_2;
> +
> +            if (strcmp(c, "1.0") == 0) version = V1_0;
> +            else if (strcmp(c, "1.2") == 0) version = V1_2;
> +            else {
> +                fprintf(stderr, "in section acs: cwmp_version has unknown 
> value (\"1.0\" or \"1.2\" expected)");
> +                goto error;
> +            }
> +
> +            acs_set_cwmp_version(version);
> +
> +                     goto next;
> +             }
>  
>  next:
>               ;
> diff --git a/src/cwmp/acs.c b/src/cwmp/acs.c
> index 5330a1a..e379049 100644
> --- a/src/cwmp/acs.c
> +++ b/src/cwmp/acs.c
> @@ -26,6 +26,7 @@ acs_init()
>       acs.hostname = NULL;
>       acs.port = 0;
>       acs.path = NULL;
> +    acs.cwmp_version = V1_2;
>  
>       FC_DEVEL_DEBUG("exit");
>  }
> @@ -48,6 +49,8 @@ acs_clean()
>       acs.port = 0;
>       if (acs.path) free(acs.path);
>       acs.path = NULL;
> +     
> +    acs.cwmp_version = V1_2;
>  
>       FC_DEVEL_DEBUG("exit");
>  }
> @@ -164,3 +167,19 @@ acs_set_path(char *c)
>       FC_DEVEL_DEBUG("exit");
>  }
>  
> +enum cwmp_version
> +acs_get_cwmp_version(void)
> +{
> +     FC_DEVEL_DEBUG("enter & exit");
> +     return acs.cwmp_version;
> +}
> +
> +void
> +acs_set_cwmp_version(enum cwmp_version version)
> +{
> +     FC_DEVEL_DEBUG("enter");
> +
> +    acs.cwmp_version = version;
> +
> +     FC_DEVEL_DEBUG("exit");
> +}
> diff --git a/src/cwmp/acs.h b/src/cwmp/acs.h
> index b502f72..3a51a46 100644
> --- a/src/cwmp/acs.h
> +++ b/src/cwmp/acs.h
> @@ -11,6 +11,8 @@
>  
>  #include <stdint.h>
>  
> +#include "cwmp.h"
> +
>  struct acs
>  {
>       char *scheme;
> @@ -19,6 +21,7 @@ struct acs
>       char *hostname;
>       uint16_t port;
>       char *path;
> +    enum cwmp_version cwmp_version;

What do you think that here we use:

        char *version;

and place there what user has written in config file?

>  };
>  
>  void acs_init();
> @@ -34,6 +37,8 @@ uint16_t acs_get_port(void);
>  void acs_set_port(char *c);
>  char * acs_get_path(void);
>  void acs_set_path(char *c);
> +enum cwmp_version acs_get_cwmp_version(void);
> +void acs_set_cwmp_version(enum cwmp_version version);
>  
>  #endif
>  
> diff --git a/src/cwmp/cwmp.h b/src/cwmp/cwmp.h
> index 6bca6b8..c3d980e 100644
> --- a/src/cwmp/cwmp.h
> +++ b/src/cwmp/cwmp.h
> @@ -12,6 +12,12 @@
>  
>  #include <libubox/uloop.h>
>  
> +enum cwmp_version
> +{
> +    V1_0 = 0,
> +    V1_2
> +};
> +
>  enum cwmp_event_code 
>  { 
>       BOOTSTRAP = 0, 
> diff --git a/src/xml/xml.c b/src/xml/xml.c
> index f0e7aa7..1972fd1 100644
> --- a/src/xml/xml.c
> +++ b/src/xml/xml.c
> @@ -88,12 +88,12 @@ xml_init(void)
>  
>       c = "http://www.w3.org/2001/XMLSchema-instance";;
>       ns.xsi_url = strdup(c);
> -     
> -     /* TODO: this works on ACS HDM, might want to chage for other ACS 
> servers */
> -//   c = "urn:dslforum-org:cwmp-1-2";
>  
> -    /* Use this for Motorola nbbs5 */
> -    c = "urn:dslforum-org:cwmp-1-0";
> +    if (acs_get_cwmp_version() == V1_2) {
> +        c = "urn:dslforum-org:cwmp-1-2";
> +    } else {
> +        c = "urn:dslforum-org:cwmp-1-0";
> +    }
>       ns.cwmp_url = strdup(c);

We should use 1.0 by default.

>       status = FC_SUCCESS;
> @@ -440,6 +440,9 @@ xml_handle_message(char *msg_in, char **msg_out)
>       char *c, *parameter_name, *parameter_value, *download_url, 
> *download_size;
>       uint8_t status, len;
>       uint16_t att_cnt;
> +    enum cwmp_version cwmp_version;
> +
> +    cwmp_version = acs_get_cwmp_version();
>  
>  #ifdef DUMMY_MODE
>       FILE *fp;
> @@ -458,29 +461,31 @@ xml_handle_message(char *msg_in, char **msg_out)
>       if (!tree_in)                                      
>               goto error;
>  
> -     /* handle cwmp:ID */
> -/*   len = snprintf(NULL, 0, "%s:%s", ns.cwmp, "ID");
> -     c = (char *) calloc((len + 1), sizeof(char));
> -     if (!c)
> -             goto error;
> -     snprintf(c, (len + 1), "%s:%s\0", ns.cwmp, "ID");
> +    if (cwmp_version == V1_2) {
> +        /* handle cwmp:ID */
> +        len = snprintf(NULL, 0, "%s:%s", ns.cwmp, "ID");
> +        c = (char *) calloc((len + 1), sizeof(char));
> +        if (!c)
> +            goto error;
> +        snprintf(c, (len + 1), "%s:%s\0", ns.cwmp, "ID");
> +
> +        busy_node = mxmlFindElement(tree_in, tree_in, c, NULL, NULL, 
> MXML_DESCEND);
> +        free(c); c = NULL;
> +        if (!busy_node)
> +            goto error;
> +
> +        busy_node = mxmlWalkNext(busy_node, tree_in, MXML_DESCEND_FIRST);
> +        if (!busy_node || !busy_node->value.text.string)
> +            goto error;
> +        tmp_node = busy_node;
> +        busy_node = mxmlFindElement(tree_out, tree_out, "cwmp:ID", NULL, 
> NULL, MXML_DESCEND);
> +        if (!busy_node)
> +            goto error;
> +        busy_node = mxmlNewText(busy_node, 0, tmp_node->value.text.string);
> +        if (!busy_node)
> +            goto error;
> +    }
>  
> -     busy_node = mxmlFindElement(tree_in, tree_in, c, NULL, NULL, 
> MXML_DESCEND);
> -     free(c); c = NULL;
> -     if (!busy_node)
> -             goto error;
> -
> -     busy_node = mxmlWalkNext(busy_node, tree_in, MXML_DESCEND_FIRST);
> -     if (!busy_node || !busy_node->value.text.string)
> -             goto error;
> -     tmp_node = busy_node;
> -     busy_node = mxmlFindElement(tree_out, tree_out, "cwmp:ID", NULL, NULL, 
> MXML_DESCEND);
> -     if (!busy_node)
> -             goto error;
> -     busy_node = mxmlNewText(busy_node, 0, tmp_node->value.text.string);
> -     if (!busy_node)
> -             goto error;
> -*/

Can you explain why you have put this only in v1.2? This is not v1.2
specific.

If you have trouble with your ACS because of this we should introduce a
new config option or better yet exclude this code with #ifdef's. And add
configuration option for your specific ACS in configure script:

./configure --enable-acs=nbbs5

Take a look how it is done for HDM, grep for ACS_HDM. You define it at
compile time using:

./configure --enable-acs=hdm

For example we have this in src/http/http.c:

94 # ifdef ACS_HDM
95         status = zstream_http_configure(http_c.stream, ZSTREAM_HTTP_COOKIES, 
1);
96 # elif ACS_MULTI
97         status = zstream_http_configure(http_c.stream, ZSTREAM_HTTP_COOKIES, 
3);
98 # endif


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