freecwmp
[Top] [All Lists]

Re: patch for the freecwmp project

To: Jonas Gorski <jonas.gorski@gmail.com>
Subject: Re: patch for the freecwmp project
From: KALLEL Mohamed <mohamed.kallel@pivasoftware.com>
Date: Wed, 29 Aug 2012 09:08:58 +0100
Cc: freecwmp@linux-mips.org, John Crispin <john@phrozen.org>
In-reply-to: <CAOiHx==ggn+ii_mr8Bc-7hgLRmwsARyuYhfNQQ-X3ERfVip6Uw@mail.gmail.com>
Original-recipient: rfc822;freecwmp@linux-mips.org
References: <50334294.9020508@pivasoftware.com> <50334314.7020807@phrozen.org> <CAOiHx==ggn+ii_mr8Bc-7hgLRmwsARyuYhfNQQ-X3ERfVip6Uw@mail.gmail.com>
Sender: freecwmp-bounce@linux-mips.org
User-agent: Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20120713 Thunderbird/14.0
Hi
Please find below my response to your remarks:


Le 28/08/2012 14:49, Jonas Gorski a écrit :
Hello Mohamed,

On 21 August 2012 10:13, John Crispin <john@phrozen.org> wrote:
Thanks for you contribution. Luka is currently on vacation so it will
take 1-2 weeks until your patch will be reviewd / merged.

Please be patient until he returns ... after a quick look at the patch i
expect this patch to get merged as it looks good !

      
On 21/08/12 10:11, KALLEL Mohamed wrote:
(...)
While there are no patch submission guidelines yet, it's probably a
good idea to orient on the kernel patch submission guidelines (see
http://www.kernel.org/doc/Documentation/SubmittingPatches, but at
least ignore the diff part). So you should at least include a
Signed-off-by line in your patch, to state that everything is yours.
as It is my first patch to the project, Please add my email contact to the comment when you commit the patch

Now onto the actual patch contents ...

diff --git a/ext/openwrt/config/freecwmp b/ext/openwrt/config/freecwmp
index 05dd3be..e6f126d 100644
--- a/ext/openwrt/config/freecwmp
+++ b/ext/openwrt/config/freecwmp
@@ -2,7 +2,8 @@ config local
 	option interface eth0
 	option port 7547
 	option ubus_socket /var/run/ubus.sock
-	option event bootstrap
+	list event bootstrap
+	list event boot
bootstrap should only be sent on the very first contact to the ACS -
you should make sure that the event gets removed after successful
delivery, so subsequent boots don't send another bootstrap.
we could add in the launcher script (freecwmp) a script which add the bootsrap event in case the acs address is changed and a boot event in case of the boot of the equipment. The both event could be added in the config (in the event list) before starting the freecwmp daemon. after the send of the event, the freecwmpd should remove the events from the config (c code) . this behaviour could be added in the future

 static bool first_run = true;
 static struct uci_context *uci_ctx;
@@ -27,13 +28,14 @@ config_free_local(void) {
 	FREE(config->local->port);
 	FREE(config->local->ubus_socket);
 	FREE(config->local);
+	event_remove_all_events();
 }

 static int
 config_init_local(void)
 {
 	struct uci_section *s;
-	struct uci_element *e;
+	struct uci_element *e,*ie;
Missing space after comma.

 	uci_foreach_element(&uci_freecwmp->sections, e) {
 		s = uci_to_section(e);
@@ -69,14 +71,20 @@ section_found:
 			goto next;
 		}

-		if (!strcmp((uci_to_option(e))->e.name, "event")) {
-			if (!strcasecmp("bootstrap", uci_to_option(e)->v.string))
-				config->local->event = BOOTSTRAP;
+		if (!strcmp((uci_to_option(e))->e.name, "event") && (uci_to_option(e))->type == UCI_TYPE_LIST) {
+			uci_foreach_element(&((uci_to_option(e))->v.list), ie)
+			{
Opening brace belongs at the end of the previous line.

+				if((ie != NULL)&&(ie->name!=NULL))
Missing spaces after if, around && and !=.

+				{
Opening brace belongs at the beginning of the previous line.

+					if (!strcasecmp("bootstrap", ie->name))
+						event_add_event(BOOTSTRAP,NULL);
Missing space after comma.


-			if (!strcasecmp("boot", uci_to_option(e)->v.string))
-				config->local->event = BOOT;
+					if (!strcasecmp("boot", ie->name))
+						event_add_event(BOOT,NULL);
Missing space after comma.


-			DD("freecwmp.@local[0].event=%s\n", uci_to_option(e)->v.string);
+					DD("freecwmp.@local[0].event=%s\n", ie->name);
+				}
+			}
 		}

 next:
diff --git a/src/config.h b/src/config.h
index 0bfc096..718d9bc 100644
--- a/src/config.h
+++ b/src/config.h
@@ -45,7 +45,6 @@ struct local {
 	char *interface;
 	char *port;
 	char *ubus_socket;
-	int event;
 };

 struct core_config {
diff --git a/src/cwmp/cwmp.c b/src/cwmp/cwmp.c
index 7ba851a..d6a0e49 100644
--- a/src/cwmp/cwmp.c
+++ b/src/cwmp/cwmp.c
@@ -12,6 +12,7 @@
 #include <libubox/uloop.h>

 #include "cwmp.h"
+#include "event.h"
 #include "external.h"
 #include "../freecwmp.h"
 #include "../config.h"
@@ -20,7 +21,7 @@

 static struct cwmp
 {
-	enum cwmp_event_code event_code;
+	struct list_head *event_list;
 	struct uloop_timeout connection_request_t;
 	struct uloop_timeout periodic_inform_t;
 	int8_t periodic_inform_enabled;
@@ -45,7 +46,7 @@ cwmp_init(void)
 	cwmp.acs_connection_required = 0;
 	cwmp.periodic_inform_enabled = 0;
 	cwmp.periodic_inform_interval = 0;
-	cwmp.event_code = config->local->event;
+	cwmp.event_list = &event_list;

 	status = cwmp_get_parameter_handler("InternetGatewayDevice.ManagementServer.PeriodicInformInterval", &c);
 	if (status == FC_SUCCESS && c) {
@@ -181,7 +182,7 @@ cwmp_periodic_inform(struct uloop_timeout *timeout)
 	if (cwmp.periodic_inform_enabled && cwmp.periodic_inform_interval) {
 		cwmp.periodic_inform_t.cb = cwmp_periodic_inform;
 		uloop_timeout_set(&cwmp.periodic_inform_t, cwmp.periodic_inform_interval * 1000);
-		cwmp.event_code = PERIODIC;
+		event_add_event(PERIODIC,NULL);
Missing space after comma.

 	}

 	if (cwmp.periodic_inform_enabled)
@@ -405,7 +406,7 @@ cwmp_connection_request(void)

 	int8_t status;

-	cwmp.event_code = CONNECTION_REQUEST;
+	event_add_event(CONNECTION_REQUEST,NULL);
Missing space after comma.

 	status = cwmp_inform();

 	FC_DEVEL_DEBUG("exit");
@@ -441,7 +442,7 @@ cwmp_add_notification(char *parameter, char *value)
 		n->value = strdup(value);
 	}

-	cwmp.event_code = VALUE_CHANGE;
+	event_add_event(VALUE_CHANGE,NULL);
Missing space after comma.

 	if (!strncmp(c, "2", 1)) {
 		cwmp_inform();
 	}
@@ -488,7 +489,7 @@ cwmp_set_parameter_write_handler(char *name, char *value)
 	}

 	if((strcmp(name, "InternetGatewayDevice.ManagementServer.URL")) == 0) {
-		cwmp.event_code = VALUE_CHANGE;
+		event_add_event(VALUE_CHANGE,NULL);
Missing space after comma.

 		cwmp.config_reload = true;
 		cwmp.acs_connection_required = 1;
 	}
@@ -657,12 +658,23 @@ done:
 }

 char *
-cwmp_get_event_code(void)
+cwmp_get_event_code(enum cwmp_get_order order)
 {
 	FC_DEVEL_DEBUG("enter");

-	char *cwmp_inform_event_code;
-	switch (cwmp.event_code) {
+	static struct list_head *ilist = NULL;
+	struct event *event;
+	char *cwmp_inform_event_code = NULL;
+
+	if (order == FIRST || ilist == NULL)
+	{
+		ilist = cwmp.event_list;
+	}
These braces are unnecessary since it is only one statement.

+	ilist = ilist->next;
+	if (ilist != cwmp.event_list)
+	{
Brace belongs at the end of the previous line.

+		event = list_entry (ilist,struct event,list);
Missing spaces after commas.

+		switch (event->code) {
 		case BOOT:
 			cwmp_inform_event_code = "1 BOOT";
 			break;
Indentation level is wrong, it should be one more tab, now that the
switch statement is one level more indented.

@@ -682,6 +694,7 @@ cwmp_get_event_code(void)
 		default:
 			cwmp_inform_event_code = "0 BOOTSTRAP";
 			break;
Up until here.

+		}
 	}

 	FC_DEVEL_DEBUG("exit");
diff --git a/src/cwmp/cwmp.h b/src/cwmp/cwmp.h
index 947539c..a1b3d8b 100644
--- a/src/cwmp/cwmp.h
+++ b/src/cwmp/cwmp.h
@@ -27,6 +27,12 @@ enum cwmp_event_code
 	AUTONOMOUS_TRANSFER_COMPLETE
 };

+enum cwmp_get_order
+{
+	FIRST = 0,
+	NEXT
+};
+
 struct notification {
 	struct list_head list;

@@ -55,7 +61,7 @@ int8_t cwmp_download_handler(char *url, char *size);
 int8_t cwmp_reboot_handler(void);
 int8_t cwmp_factory_reset_handler(void);
 int8_t cwmp_reload_changes(void);
-char * cwmp_get_event_code(void);
+char * cwmp_get_event_code(enum cwmp_get_order);
Generally I don't really like this interface. Events might be
read/modified/added concurrently, and using this interface will make
it hard to make it thread-safe. I'd rather see the list directly
accessed, and then add a mutex for protecting accesses to it.
I suggest to keep for the moment the interface as I developed till the event behaviour find his way in the source code. and I m agree with you that the event behaviour is a bit complicated (Multiple event, single event, more than 1 event in the inform message, pssibility to have 2 event list at the same time in the C code, a list which used in the current session with acs, and another list which used as queue of events, possibility to add mutex to manage events : session events and queue events...) so that why I have added 2 files for  the events(event.c and event.h)

 int cwmp_get_retry_count(void);

 #endif
diff --git a/src/cwmp/event.c b/src/cwmp/event.c
index e69de29..561d413 100644
--- a/src/cwmp/event.c
+++ b/src/cwmp/event.c
@@ -0,0 +1,33 @@
+#include "event.h"
+#include "cwmp.h"
+#include "../freecwmp.h"
+LIST_HEAD(event_list);
+
+void
+event_add_event(uint8_t code, char *commandKey)
+{
+	struct event *event;
+
+	event = calloc(1,sizeof(struct event));
Missing space after comma.

+	event->code = code;
+	event->commandKey = commandKey;
You should strdup the commandKey and then free it in remove. That way
the caller does not have to keep it until the event got sent and is
free to free the memory immediately.
This behaviour could be integrated in the future when starting playing with commandKeys.

+	list_add (&(event->list), &(event_list));
+
+	FC_DEVEL_DEBUG("exit");
+}
Again, this should be protected against concurrent access. Also you
should make sure that the basic events like BOOTSTRAP, BOOT, PERIODIC
INFORM are present at most once, as mandated by the standard.
Should be added in the future in the event management behavior (event.c file).

+
+void
+event_remove_all_events()
+{
+	struct event *event;
+
+	FC_DEVEL_DEBUG("enter");
+
+	while (event_list.next != &event_list)
+	{
Opening brace at the end of the previous line.

+		event = list_entry (event_list.next,struct event, list);
No space before (, missing spaces after commas.

+		list_del(&event->list);
+		free(event);
+	}
+	FC_DEVEL_DEBUG("exit");
+}
diff --git a/src/cwmp/event.h b/src/cwmp/event.h
index e69de29..07afa8e 100644
--- a/src/cwmp/event.h
+++ b/src/cwmp/event.h
@@ -0,0 +1,18 @@
+#ifndef _FREECWMP_EVENT_H__
+#define _FREECWMP_EVENT_H__
+
+#include <stdint.h>
+#include <libubox/uloop.h>
+
+struct event
+{
+	struct list_head list;
+	char *commandKey;
+	uint8_t code;
+};
+
+void event_add_event(uint8_t code, char *commandKey);
+void event_remove_all_events();
+
+extern struct list_head event_list;
+#endif
diff --git a/src/cwmp/messages.h b/src/cwmp/messages.h
index 046fc82..eea86d1 100644
--- a/src/cwmp/messages.h
+++ b/src/cwmp/messages.h
@@ -29,11 +29,7 @@
 			"<ProductClass/>"									\
 			"<SerialNumber/>"									\
 		"</DeviceId>"											\
-		"<Event soap_enc:arrayType=\"cwmp:EventStruct[1]\">"						\
-			"<EventStruct>"										\
-				"<EventCode/>"									\
-				"<CommandKey/>"									\
-			"</EventStruct>"									\
+		"<Event soap_enc:arrayType=\"cwmp:EventStruct[0]\">"						\
 		"</Event>"											\
 		"<MaxEnvelopes>1</MaxEnvelopes>"								\
 		"<CurrentTime/>"										\
@@ -88,6 +84,11 @@
 "</soap_env:Body>"												\
 "</soap_env:Envelope>"

+#define CWMP_INFORM_EVENT_STRUCT \
+"<EventStruct>"			\
+	"<EventCode/>"		\
+	"<CommandKey/>"		\
+"</EventStruct>"

 #define CWMP_RESPONSE_MESSAGE \
 "<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\"?>"		\
diff --git a/src/xml/xml.c b/src/xml/xml.c
index 05424a5..c05cfc0 100644
--- a/src/xml/xml.c
+++ b/src/xml/xml.c
@@ -124,6 +124,40 @@ xml_exit(void)
 }

 int8_t
+xml_prepare_events_inform (mxml_node_t *tree)
+{
+	mxml_node_t *node, *event_node, *code_node;
+	char *c, buf[32];
The buffer can be smaller, as strlen("cwmp:EventStruct[%u]") + 1 = 22.

+	uint8_t n = 0;
+
+
+	node = mxmlFindElement(tree, tree, "Event", NULL, NULL, MXML_DESCEND);
+
+	for (c = cwmp_get_event_code(FIRST); c!= NULL; c =  cwmp_get_event_code(NEXT))
+	{
Opening brace at the end of the previous line, please.
+		event_node	= mxmlLoadString(NULL, CWMP_INFORM_EVENT_STRUCT, MXML_NO_CALLBACK);
No need to align the =; replace the tab before it with a simple space.

+		if (!event_node)
+			return 0;
You should return FC_ERROR here (0 is FC_SUCCESS).

+		code_node	= mxmlFindElement(event_node, event_node, "EventCode", NULL, NULL, MXML_DESCEND);
See above.

+		if (!code_node)
+			return 0;
See above.

+		code_node	= mxmlNewText(code_node, 0, c);
See above.

+		if (!code_node)
+			return 0;
See above.

+		mxmlAdd (node,MXML_ADD_AFTER,MXML_ADD_TO_PARENT,event_node);
Remove the space before the (, and add spaces after the commas.

+		n++;
Don't you also have to set the command key if there is one? Where did
that one go?

Also generally wouldn't it be easier to just add the EventStruct node
with its two subnodes through mxml calls, instead of loading the
string and parsing it for every node?

Also you should check that you have at most 64 events (the standard
allows only 64).

+	}
+
+	if (n)
+	{
Opening brace at end of previous line.

+		sprintf (buf,"cwmp:EventStruct[%u]",n);
Remove the space before parenthesis, and add spaces after commas.

+		mxmlElementSetAttr(node, "soap_enc:arrayType", buf );	/* I - Attribute value */
+	}
+
+	return 1;
You should return FC_SUCCESS here (which is 0). Also you are missing
FC_DEVEL_DEBUG("enter/exit") calls.

+}
+
+int8_t
 xml_prepare_inform_message(char **msg_out)
 {
 	FC_DEVEL_DEBUG("enter");
@@ -178,11 +212,7 @@ xml_prepare_inform_message(char **msg_out)
 	if (!busy_node)
 		goto error;

-	busy_node = mxmlFindElement(tree, tree, "EventCode", NULL, NULL, MXML_DESCEND);
-	if (!busy_node)
-		goto error;
-	busy_node = mxmlNewText(busy_node, 0, cwmp_get_event_code());
-	if (!busy_node)
+	if (!xml_prepare_events_inform (tree))
The check should be if (... != FC_SUCCESS)

 		goto error;

 	busy_node = mxmlFindElement(tree, tree, "CurrentTime", NULL, NULL, MXML_DESCEND);

Regards
Jonas

Mohamed KALLEL
Regards

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