freecwmp
[Top] [All Lists]

Re: [PATCH] freecwmp: make cwmp version configurable

To: Luka Perkov <freecwmp@lukaperkov.net>
Subject: Re: [PATCH] freecwmp: make cwmp version configurable
From: Kaspar Schleiser <kaspar@schleiser.de>
Date: Wed, 22 Feb 2012 17:35:50 +0100
Cc: freecwmp@linux-mips.org
In-reply-to: <20120222152329.GA25852@w500.iskon.local>
Original-recipient: rfc822;freecwmp@linux-mips.org
References: <1329917025-15282-1-git-send-email-kaspar@schleiser.de> <20120222152329.GA25852@w500.iskon.local>
Sender: freecwmp-bounce@linux-mips.org
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.1) Gecko/20120212 Thunderbird/10.0.1
Hey Luka,

On 02/22/2012 04:23 PM, Luka Perkov wrote:
  * use tabs instead of spaces
  * avoid trailing wihite spaces
Totally agree. Sorry for not noticing.

On Wed, Feb 22, 2012 at 02:23:45PM +0100, Kaspar Schleiser wrote:
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.
Ok. Didn't want to change default behaviour.

Specs say, use the newest one you support, but be able to read the version out of the server's response...

Do you have an idea of where the actual differences between the standards are? I just noticed that 1.2 adds some rpc calls like for module support. If the client doesn't behave differently when not asked to, maybe always using 1.2 is ok.

I had this implemented just for the expected version urn sent by the server... If we can handle both versions there, maybe we don't even need configuration.

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?
I thought that the version would set a specific but well defined behaviour of the client. Passing the string would require a strcmp on all checks which seems inefficient to me.

Compare

if (acs.version == V1_2)
with

if(strcmp(acs.version, "1.2") == 0)

For the urn string, for version "1.0" we have to send "...1-0". I don't think this should be user configurable. Either we speak 1.0 which implies sending "1-0" (and maybe other things), or we speak 1.2 which implies "1-2". If a user wants to get that deep, he can easily hack the code...

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

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.
I'll rework it. Hope you get well soon!

+    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?
See above. I had it implemented with char*, but then all the checks seemed ugly. Your call! I don't really mind.

+    if (cwmp_version == V1_2) {
...
+    }

Can you explain why you have put this only in v1.2? This is not v1.2
specific.
Well, I just blindly assumed that this was an 1.0 vs 1.2 issue without consulting the specs. ;) I'll gladly remove it from the version support patch.

I just checked, the specs say that *if* the server sends cwmp:ID, this id *must* be used in any response to that request. I'll look into making the client behave like that, maybe we don't need special care for nbbs.

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
I hoped to not need any (non-generic) server specific code, but I'll integrate it this way next time.

Kaspar



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