From mohamed.kallel@pivasoftware.com Tue Aug 21 10:11:15 2012 Received: with ECARTIS (v1.0.0; list freecwmp); Tue, 21 Aug 2012 10:11:24 +0200 (CEST) Received: from moutng.kundenserver.de ([212.227.126.171]:56020 "EHLO moutng.kundenserver.de" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S1903520Ab2HUILP (ORCPT ); Tue, 21 Aug 2012 10:11:15 +0200 Received: from [127.0.0.1] ([197.27.44.119]) by mrelayeu.kundenserver.de (node=mrbap3) with ESMTP (Nemesis) id 0Le4z6-1TTEXc1Rt6-00psBw; Tue, 21 Aug 2012 10:11:08 +0200 Message-ID: <50334294.9020508@pivasoftware.com> Date: Tue, 21 Aug 2012 09:11:00 +0100 From: KALLEL Mohamed User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20120713 Thunderbird/14.0 MIME-Version: 1.0 To: luka.perkov@lukaperkov.net, freecwmp@linux-mips.org Subject: patch for the freecwmp project Content-Type: multipart/mixed; boundary="------------010102080906020205090204" X-Provags-ID: V02:K0:INsWCrntfgxJXGC8TVTklAA6uzbsCR2DqG9bwfKUgSf zVBGWXgUafflp7VGx1VD/XoU22pUchb84yuF9UgKiHQDR9123/ otXr1epzVB5SGG0mcG/8SLF2FUKRKCDFPBIOeB5jOY/gVWHBP2 ymIw75zGiwP0ppPCp0K6Opm2AmFzWn3UUohyZAONRWYtEP0xBt 3Ta8GQOdaFRVTU+sWvUXXbnOLPjK200Jp/sFOtqDWoCQJ7ywUa Y8EYzZFg4doJhkNBzn4qW7n8abYdKQ/rJruEYL2zfMC9WDQP68 +mezTG0ThbdaCxqC0ZStlm9oTzdO0tlk7/jIHlUT95yr43/jpX 8hWX4kOjexr7RJDMYf5o62oOnKhdOQ3jWMsUhQhE9 Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s freecwmp"> (uid 0) X-Orcpt: rfc822;freecwmp@linux-mips.org Original-Recipient: rfc822;freecwmp@linux-mips.org X-archive-position: 51 X-ecartis-version: Ecartis v1.0.0 Sender: freecwmp-bounce@linux-mips.org Errors-to: freecwmp-bounce@linux-mips.org X-original-sender: mohamed.kallel@pivasoftware.com Precedence: bulk X-list: freecwmp This is a multi-part message in MIME format. --------------010102080906020205090204 Content-Type: multipart/alternative; boundary="------------050307050109060007090903" --------------050307050109060007090903 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Hi Luka Perkov I'm Mohamed KALLEL and I'm interested in your freecwmp project. And I want to contribute in the development with the following patch (patch attached with this email). The patch allow to support many events in the inform message. the old behavior support only one event in the inform message this patch allow to add the boot event (with the bootstarp event) in the inform message. the patch is based on the revision: *6d074ab* Mohamed KALLEL --------------050307050109060007090903 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit

Hi Luka Perkov

I'm Mohamed KALLEL and I'm interested in your freecwmp project.
And I want to contribute in the development with the following patch (patch attached with this email).

The patch allow to support many events in the inform message. the old behavior support only one event in the inform message
this patch allow to add the boot event (with the bootstarp event) in the inform message.
the patch is based on the revision: 6d074ab

Mohamed KALLEL

--------------050307050109060007090903-- --------------010102080906020205090204 Content-Type: text/plain; charset=windows-1256; name="patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="patch" diff --git a/bin/Makefile.am b/bin/Makefile.am index aa4db87..d7abfe2 100644 --- a/bin/Makefile.am +++ b/bin/Makefile.am @@ -7,6 +7,8 @@ freecwmpd_SOURCES = \ ../src/config.c \ ../src/cwmp/cwmp.h \ ../src/cwmp/cwmp.c \ + ../src/cwmp/event.h \ + ../src/cwmp/event.c \ ../src/cwmp/external.h \ ../src/cwmp/external.c \ ../src/http/b64.h \ 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 config acs option scheme http diff --git a/src/config.c b/src/config.c index ad7a86e..abc621c 100644 --- a/src/config.c +++ b/src/config.c @@ -12,6 +12,7 @@ #include "config.h" #include "cwmp/cwmp.h" +#include "cwmp/event.h" 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; 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) + { + if((ie != NULL)&&(ie->name!=NULL)) + { + if (!strcasecmp("bootstrap", ie->name)) + event_add_event(BOOTSTRAP,NULL); - if (!strcasecmp("boot", uci_to_option(e)->v.string)) - config->local->event = BOOT; + if (!strcasecmp("boot", ie->name)) + event_add_event(BOOT,NULL); - 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 #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); } 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); 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); 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); 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; + } + ilist = ilist->next; + if (ilist != cwmp.event_list) + { + event = list_entry (ilist,struct event,list); + switch (event->code) { case BOOT: cwmp_inform_event_code = "1 BOOT"; break; @@ -682,6 +694,7 @@ cwmp_get_event_code(void) default: cwmp_inform_event_code = "0 BOOTSTRAP"; break; + } } 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); 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)); + event->code = code; + event->commandKey = commandKey; + list_add (&(event->list), &(event_list)); + + FC_DEVEL_DEBUG("exit"); +} + +void +event_remove_all_events() +{ + struct event *event; + + FC_DEVEL_DEBUG("enter"); + + while (event_list.next != &event_list) + { + event = list_entry (event_list.next,struct event, list); + 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 +#include + +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 @@ "" \ "" \ "" \ - "" \ - "" \ - "" \ - "" \ - "" \ + "" \ "" \ "1" \ "" \ @@ -88,6 +84,11 @@ "" \ "" +#define CWMP_INFORM_EVENT_STRUCT \ +"" \ + "" \ + "" \ +"" #define CWMP_RESPONSE_MESSAGE \ "" \ 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]; + 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)) + { + event_node = mxmlLoadString(NULL, CWMP_INFORM_EVENT_STRUCT, MXML_NO_CALLBACK); + if (!event_node) + return 0; + code_node = mxmlFindElement(event_node, event_node, "EventCode", NULL, NULL, MXML_DESCEND); + if (!code_node) + return 0; + code_node = mxmlNewText(code_node, 0, c); + if (!code_node) + return 0; + mxmlAdd (node,MXML_ADD_AFTER,MXML_ADD_TO_PARENT,event_node); + n++; + } + + if (n) + { + sprintf (buf,"cwmp:EventStruct[%u]",n); + mxmlElementSetAttr(node, "soap_enc:arrayType", buf ); /* I - Attribute value */ + } + + return 1; +} + +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)) goto error; busy_node = mxmlFindElement(tree, tree, "CurrentTime", NULL, NULL, MXML_DESCEND); --------------010102080906020205090204-- From john@phrozen.org Tue Aug 21 10:14:29 2012 Received: with ECARTIS (v1.0.0; list freecwmp); Tue, 21 Aug 2012 10:14:33 +0200 (CEST) Received: from nbd.name ([46.4.11.11]:55509 "EHLO nbd.name" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S1901167Ab2HUIO3 (ORCPT ); Tue, 21 Aug 2012 10:14:29 +0200 Message-ID: <50334314.7020807@phrozen.org> Date: Tue, 21 Aug 2012 10:13:08 +0200 From: John Crispin User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.24) Gecko/20111114 Icedove/3.1.16 MIME-Version: 1.0 To: KALLEL Mohamed CC: freecwmp@linux-mips.org Subject: Re: patch for the freecwmp project References: <50334294.9020508@pivasoftware.com> In-Reply-To: <50334294.9020508@pivasoftware.com> X-Enigmail-Version: 1.1.2 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s freecwmp"> (uid 0) X-Orcpt: rfc822;freecwmp@linux-mips.org Original-Recipient: rfc822;freecwmp@linux-mips.org X-archive-position: 52 X-ecartis-version: Ecartis v1.0.0 Sender: freecwmp-bounce@linux-mips.org Errors-to: freecwmp-bounce@linux-mips.org X-original-sender: john@phrozen.org Precedence: bulk X-list: freecwmp On 21/08/12 10:11, KALLEL Mohamed wrote: > Hi Luka Perkov > > I'm Mohamed KALLEL and I'm interested in your freecwmp project. > And I want to contribute in the development with the following patch > (patch attached with this email). > > The patch allow to support many events in the inform message. the old > behavior support only one event in the inform message > this patch allow to add the boot event (with the bootstarp event) in the > inform message. > the patch is based on the revision: *6d074ab* > > Mohamed KALLEL > Hi Mohamed, 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 ! Thanks, John From freecwmp@lukaperkov.net Tue Aug 28 14:28:36 2012 Received: with ECARTIS (v1.0.0; list freecwmp); Tue, 28 Aug 2012 14:28:40 +0200 (CEST) Received: from mail-bk0-f49.google.com ([209.85.214.49]:50201 "EHLO mail-bk0-f49.google.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S1903718Ab2H1M2g (ORCPT ); Tue, 28 Aug 2012 14:28:36 +0200 Received: by bkcji2 with SMTP id ji2so1701964bkc.36 for ; Tue, 28 Aug 2012 05:28:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent:x-gm-message-state; bh=WszsKUQ/dug1uDbfGIfFrCHIDCQ/J6jdVG60b3tNIAs=; b=g1hfXAXNfbEq9cRII+P2mpyCrGoX38sYKiEWXu1uCbIzv4M9Ce13Jh9OrsVAZ0L+4O ImhrwNDd4MG7mS9Wq1HFCmeS2/gESjp5pYQzgSrL+uFQyS2vpUF7BFRQmxsM7wYcx7oh JC/L2zijcIXXutIG1m8zyVLvlMStO4JRk5UEZU1HeVu9EMfrD6cRpAeynyxxA4uBCbYj JmPgW9wjY3gsc74izuupCRlJ8a2mCQq8AHX1IKN3GljANsEn8AUkUKhnOHZ6T8heuAYG hD6ci0H3Ji3uvt1j0IpJ/7mjj34d6zkjUS/s/y6QXR6xOm99F9h8uEmJEyZEXuYtQUy8 owow== Received: by 10.204.130.209 with SMTP id u17mr4827027bks.35.1346156910336; Tue, 28 Aug 2012 05:28:30 -0700 (PDT) Received: from localhost (213-191-157-42.dhcp.iskon.hr. [213.191.157.42]) by mx.google.com with ESMTPS id fu8sm12652322bkc.5.2012.08.28.05.28.28 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 28 Aug 2012 05:28:29 -0700 (PDT) Date: Tue, 28 Aug 2012 14:28:41 +0200 From: Luka Perkov To: KALLEL Mohamed Cc: freecwmp@linux-mips.org Subject: Re: patch for the freecwmp project Message-ID: <20120828122841.GA8612@w500.iskon.local> Mail-Followup-To: KALLEL Mohamed , freecwmp@linux-mips.org References: <50334294.9020508@pivasoftware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50334294.9020508@pivasoftware.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Gm-Message-State: ALoCoQlKharaDYgOIvHUTbzgIiq6qGC1S8Wi67sIjB9Z3xsitK8jYYP0bpETZjDMJV0NwGeAo3EA Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s freecwmp"> (uid 0) X-Orcpt: rfc822;freecwmp@linux-mips.org Original-Recipient: rfc822;freecwmp@linux-mips.org X-archive-position: 53 X-ecartis-version: Ecartis v1.0.0 Sender: freecwmp-bounce@linux-mips.org Errors-to: freecwmp-bounce@linux-mips.org X-original-sender: freecwmp@lukaperkov.net Precedence: bulk X-list: freecwmp Hi Mohamed, On Tue, Aug 21, 2012 at 09:11:00AM +0100, KALLEL Mohamed wrote: > I'm Mohamed KALLEL and I'm interested in your freecwmp project. Welcome aboard! > And I want to contribute in the development with the following patch > (patch attached with this email). Thank you. I'll try to merge it this week, for now I'll give some quick comments bellow. > The patch allow to support many events in the inform message. the > old behavior support only one event in the inform message > this patch allow to add the boot event (with the bootstarp event) in > the inform message. > the patch is based on the revision: *6d074ab* > > Mohamed KALLEL > > > diff --git a/bin/Makefile.am b/bin/Makefile.am > index aa4db87..d7abfe2 100644 > --- a/bin/Makefile.am > +++ b/bin/Makefile.am > @@ -7,6 +7,8 @@ freecwmpd_SOURCES = \ > ../src/config.c \ > ../src/cwmp/cwmp.h \ > ../src/cwmp/cwmp.c \ > + ../src/cwmp/event.h \ > + ../src/cwmp/event.c \ > ../src/cwmp/external.h \ > ../src/cwmp/external.c \ > ../src/http/b64.h \ > 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 I think we could leave this out and by default if there is no event list we use this two options (bootstrap and boot). > config acs > option scheme http > diff --git a/src/config.c b/src/config.c > index ad7a86e..abc621c 100644 > --- a/src/config.c > +++ b/src/config.c > @@ -12,6 +12,7 @@ > > #include "config.h" > #include "cwmp/cwmp.h" > +#include "cwmp/event.h" > > 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; > > 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) > + { > + if((ie != NULL)&&(ie->name!=NULL)) > + { > + if (!strcasecmp("bootstrap", ie->name)) > + event_add_event(BOOTSTRAP,NULL); > > - if (!strcasecmp("boot", uci_to_option(e)->v.string)) > - config->local->event = BOOT; > + if (!strcasecmp("boot", ie->name)) > + event_add_event(BOOT,NULL); > > - DD("freecwmp.@local[0].event=%s\n", uci_to_option(e)->v.string); > + DD("freecwmp.@local[0].event=%s\n", ie->name); I'm not sure if this output will be the same as the one from uci. I'll double check. > + } > + } > } > > 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 > > #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); > } > > 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); > 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); > 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); > 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) > + { Coding style tip: if (order == FIRST || ilist == NULL) { That goes for all your if's and for's... > + ilist = cwmp.event_list; > + } > + ilist = ilist->next; > + if (ilist != cwmp.event_list) > + { > + event = list_entry (ilist,struct event,list); > + switch (event->code) { > case BOOT: > cwmp_inform_event_code = "1 BOOT"; > break; > @@ -682,6 +694,7 @@ cwmp_get_event_code(void) > default: > cwmp_inform_event_code = "0 BOOTSTRAP"; > break; > + } > } > > 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); > 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 GPLv2 license header is missing... Also I'll see if this can go in some existing file. > @@ -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) Please name your variables like this command_key instead commandKey. > +{ > + struct event *event; > + > + event = calloc(1,sizeof(struct event)); > + event->code = code; > + event->commandKey = commandKey; > + list_add (&(event->list), &(event_list)); > + > + FC_DEVEL_DEBUG("exit"); > +} > + > +void > +event_remove_all_events() > +{ > + struct event *event; > + > + FC_DEVEL_DEBUG("enter"); I'm doing some cleanup so I'll get rid of FC_DEVEL_DEBUG in the near future. It's ok for now, I just wanted to give you heads up. > + > + while (event_list.next != &event_list) > + { > + event = list_entry (event_list.next,struct event, list); > + 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 @@ GPLv2 license header is missing... > +#ifndef _FREECWMP_EVENT_H__ > +#define _FREECWMP_EVENT_H__ > + > +#include > +#include > + > +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 @@ > "" \ > "" \ > "" \ > - "" \ > - "" \ > - "" \ > - "" \ > - "" \ > + "" \ > "" \ > "1" \ > "" \ > @@ -88,6 +84,11 @@ > "" \ > "" > > +#define CWMP_INFORM_EVENT_STRUCT \ > +"" \ > + "" \ > + "" \ > +"" > > #define CWMP_RESPONSE_MESSAGE \ > "" \ > 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]; > + 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)) > + { > + event_node = mxmlLoadString(NULL, CWMP_INFORM_EVENT_STRUCT, MXML_NO_CALLBACK); > + if (!event_node) > + return 0; > + code_node = mxmlFindElement(event_node, event_node, "EventCode", NULL, NULL, MXML_DESCEND); > + if (!code_node) > + return 0; > + code_node = mxmlNewText(code_node, 0, c); > + if (!code_node) > + return 0; > + mxmlAdd (node,MXML_ADD_AFTER,MXML_ADD_TO_PARENT,event_node); > + n++; > + } > + > + if (n) > + { > + sprintf (buf,"cwmp:EventStruct[%u]",n); > + mxmlElementSetAttr(node, "soap_enc:arrayType", buf ); /* I - Attribute value */ > + } > + > + return 1; > +} > + > +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)) > goto error; > > busy_node = mxmlFindElement(tree, tree, "CurrentTime", NULL, NULL, MXML_DESCEND); That would be it for now. I'll try and merge your patch this week. I would also like to invite you to #freecwmp IRC channel on freenode. Regards, Luka From jonas.gorski@gmail.com Tue Aug 28 15:50:21 2012 Received: with ECARTIS (v1.0.0; list freecwmp); Tue, 28 Aug 2012 15:50:29 +0200 (CEST) Received: from mail-ob0-f177.google.com ([209.85.214.177]:52250 "EHLO mail-ob0-f177.google.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S1903750Ab2H1NuV (ORCPT ); Tue, 28 Aug 2012 15:50:21 +0200 Received: by obbta17 with SMTP id ta17so11079091obb.36 for ; Tue, 28 Aug 2012 06:50:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=rlbVSThWHHKzKFXv89OSnJjSwgCDvm5Z5gGeS01/wM0=; b=B7tcDsN2oaZdo/ZToQPbDaMRGY4Hj2AiqnlQzWk2UBQLXjLlr4lgDRhA4TtfHL0KKo F/n2fAic+22BkzZQSq5U7XlGl2W8ppSVyhNzpYzafPXJZKSEwDWWwtrreipvkJD3p1JU kr7bG0ne58XS9ZU+3hIvCtMNK6IeczDnzHbVdW3gFAuir8sKlS8mEjbFy5SdtBlNcDxK cXUHSMr9x9juue3t4d63F+6JAlpRjXBowGADKmwhms8KbjeKZaYcQS5trfMQ1fVObq8v UZ4RZsuwKvYnBTCpACctMGs/4igjN2zFto6F8GuHE4Em/iB7b7CxQStPp88iN1B0tYLJ IcTg== Received: by 10.60.22.33 with SMTP id a1mr12428397oef.141.1346161815494; Tue, 28 Aug 2012 06:50:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.76.3.15 with HTTP; Tue, 28 Aug 2012 06:49:55 -0700 (PDT) In-Reply-To: <50334314.7020807@phrozen.org> References: <50334294.9020508@pivasoftware.com> <50334314.7020807@phrozen.org> From: Jonas Gorski Date: Tue, 28 Aug 2012 15:49:55 +0200 Message-ID: Subject: Re: patch for the freecwmp project To: KALLEL Mohamed Cc: freecwmp@linux-mips.org, John Crispin Content-Type: text/plain; charset=UTF-8 Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s freecwmp"> (uid 0) X-Orcpt: rfc822;freecwmp@linux-mips.org Original-Recipient: rfc822;freecwmp@linux-mips.org X-archive-position: 54 X-ecartis-version: Ecartis v1.0.0 Sender: freecwmp-bounce@linux-mips.org Errors-to: freecwmp-bounce@linux-mips.org X-original-sender: jonas.gorski@gmail.com Precedence: bulk X-list: freecwmp Hello Mohamed, On 21 August 2012 10:13, John Crispin 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. 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. > 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 > > #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. > 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. > + 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. > + > +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 > +#include > + > +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 @@ > "" \ > "" \ > "" \ > - "" \ > - "" \ > - "" \ > - "" \ > - "" \ > + "" \ > "" \ > "1" \ > "" \ > @@ -88,6 +84,11 @@ > "" \ > "" > > +#define CWMP_INFORM_EVENT_STRUCT \ > +"" \ > + "" \ > + "" \ > +"" > > #define CWMP_RESPONSE_MESSAGE \ > "" \ > 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 From mohamed.kallel@pivasoftware.com Wed Aug 29 09:51:19 2012 Received: with ECARTIS (v1.0.0; list freecwmp); Wed, 29 Aug 2012 09:51:24 +0200 (CEST) Received: from moutng.kundenserver.de ([212.227.17.8]:49561 "EHLO moutng.kundenserver.de" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S1903052Ab2H2HvS (ORCPT ); Wed, 29 Aug 2012 09:51:18 +0200 Received: from [127.0.0.1] ([41.224.250.29]) by mrelayeu.kundenserver.de (node=mreu4) with ESMTP (Nemesis) id 0LwjiG-1ThrWg1zw0-016RRX; Wed, 29 Aug 2012 09:51:13 +0200 Message-ID: <503DC9E9.6030506@pivasoftware.com> Date: Wed, 29 Aug 2012 08:51:05 +0100 From: KALLEL Mohamed User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20120713 Thunderbird/14.0 MIME-Version: 1.0 To: freecwmp@linux-mips.org Subject: Re: patch for the freecwmp project References: <50334294.9020508@pivasoftware.com> <20120828122841.GA8612@w500.iskon.local> In-Reply-To: <20120828122841.GA8612@w500.iskon.local> Content-Type: multipart/alternative; boundary="------------070001010803020707080504" X-Provags-ID: V02:K0:mExCZJpVKo5c98VY6iXdZoO3hMMOrVff4YiJEyfUY6S cosr+KYLR8yYLAMPEuquU7En4OLef3Xpm11med8dWfXrpmIHQ9 A6BupvbHaUexCSGH5X2Z9OJzWX5vxCaFaN+fQUMW4B3NLaU4ZV PTbH0BGhqRIQGQmDMsHFpQpAZd8o4ZSCnKLBnZKupdaPs5UNQv mHyzUhoEUw41rqvWeqZ7wKfclu3U5nldemtW2hHZw6fQd0QT6b AmLXt5Ijnf+AyoWlXQEYYEz1GW+YnFxyX+tdq+qzmZ64kd5Rhn 3fNzWH2aFUedXPTrQAbycGY6n6s1vmEyHLU8krhMZvQYnm4Re9 qG6H9CTCdNzysSl37S0bz+b19sAOaDXZHxmF/ztJN Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s freecwmp"> (uid 0) X-Orcpt: rfc822;freecwmp@linux-mips.org Original-Recipient: rfc822;freecwmp@linux-mips.org X-archive-position: 55 X-ecartis-version: Ecartis v1.0.0 Sender: freecwmp-bounce@linux-mips.org Errors-to: freecwmp-bounce@linux-mips.org X-original-sender: mohamed.kallel@pivasoftware.com Precedence: bulk X-list: freecwmp This is a multi-part message in MIME format. --------------070001010803020707080504 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Hi It's pleasure to me contribute to your project and thank you for accepting the path Please , see here after my response to your comments. Le 28/08/2012 13:28, Luka Perkov a écrit : > Hi Mohamed, > > On Tue, Aug 21, 2012 at 09:11:00AM +0100, KALLEL Mohamed wrote: >> I'm Mohamed KALLEL and I'm interested in your freecwmp project. > Welcome aboard! > >> And I want to contribute in the development with the following patch >> (patch attached with this email). > Thank you. I'll try to merge it this week, for now I'll give some quick > comments bellow. > >> The patch allow to support many events in the inform message. the >> old behavior support only one event in the inform message >> this patch allow to add the boot event (with the bootstarp event) in >> the inform message. >> the patch is based on the revision: *6d074ab* >> >> Mohamed KALLEL >> >> >> diff --git a/bin/Makefile.am b/bin/Makefile.am >> index aa4db87..d7abfe2 100644 >> --- a/bin/Makefile.am >> +++ b/bin/Makefile.am >> @@ -7,6 +7,8 @@ freecwmpd_SOURCES = \ >> ../src/config.c \ >> ../src/cwmp/cwmp.h \ >> ../src/cwmp/cwmp.c \ >> + ../src/cwmp/event.h \ >> + ../src/cwmp/event.c \ >> ../src/cwmp/external.h \ >> ../src/cwmp/external.c \ >> ../src/http/b64.h \ >> 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 > I think we could leave this out and by default if there is no event list > we use this two options (bootstrap and boot). 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 > >> config acs >> option scheme http >> diff --git a/src/config.c b/src/config.c >> index ad7a86e..abc621c 100644 >> --- a/src/config.c >> +++ b/src/config.c >> @@ -12,6 +12,7 @@ >> >> #include "config.h" >> #include "cwmp/cwmp.h" >> +#include "cwmp/event.h" >> >> 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; >> >> 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) >> + { >> + if((ie != NULL)&&(ie->name!=NULL)) >> + { >> + if (!strcasecmp("bootstrap", ie->name)) >> + event_add_event(BOOTSTRAP,NULL); >> >> - if (!strcasecmp("boot", uci_to_option(e)->v.string)) >> - config->local->event = BOOT; >> + if (!strcasecmp("boot", ie->name)) >> + event_add_event(BOOT,NULL); >> >> - DD("freecwmp.@local[0].event=%s\n", uci_to_option(e)->v.string); >> + DD("freecwmp.@local[0].event=%s\n", ie->name); > I'm not sure if this output will be the same as the one from uci. I'll > double check. I Know that the output is not the same as the one from uci. but this the quick solution I found for this kind of debug message. (It's just a debug message, so that why I did not look for a solution this) > >> + } >> + } >> } >> >> 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 >> >> #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); >> } >> >> 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); >> 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); >> 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); >> 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) >> + { > Coding style tip: > > if (order == FIRST || ilist == NULL) { > > That goes for all your if's and for's... > >> + ilist = cwmp.event_list; >> + } >> + ilist = ilist->next; >> + if (ilist != cwmp.event_list) >> + { >> + event = list_entry (ilist,struct event,list); >> + switch (event->code) { >> case BOOT: >> cwmp_inform_event_code = "1 BOOT"; >> break; >> @@ -682,6 +694,7 @@ cwmp_get_event_code(void) >> default: >> cwmp_inform_event_code = "0 BOOTSTRAP"; >> break; >> + } >> } >> >> 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); >> 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 > GPLv2 license header is missing... > > Also I'll see if this can go in some existing file. I suggest to keep both file untill we finish the event behaviours . Because, as indicated in the standard, the management of the event 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, 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...) > >> @@ -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) > Please name your variables like this command_key instead commandKey. > >> +{ >> + struct event *event; >> + >> + event = calloc(1,sizeof(struct event)); >> + event->code = code; >> + event->commandKey = commandKey; >> + list_add (&(event->list), &(event_list)); >> + >> + FC_DEVEL_DEBUG("exit"); >> +} >> + >> +void >> +event_remove_all_events() >> +{ >> + struct event *event; >> + >> + FC_DEVEL_DEBUG("enter"); > I'm doing some cleanup so I'll get rid of FC_DEVEL_DEBUG in the near > future. It's ok for now, I just wanted to give you heads up. > >> + >> + while (event_list.next != &event_list) >> + { >> + event = list_entry (event_list.next,struct event, list); >> + 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 @@ > GPLv2 license header is missing... > >> +#ifndef _FREECWMP_EVENT_H__ >> +#define _FREECWMP_EVENT_H__ >> + >> +#include >> +#include >> + >> +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 @@ >> "" \ >> "" \ >> "" \ >> - "" \ >> - "" \ >> - "" \ >> - "" \ >> - "" \ >> + "" \ >> "" \ >> "1" \ >> "" \ >> @@ -88,6 +84,11 @@ >> "" \ >> "" >> >> +#define CWMP_INFORM_EVENT_STRUCT \ >> +"" \ >> + "" \ >> + "" \ >> +"" >> >> #define CWMP_RESPONSE_MESSAGE \ >> "" \ >> 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]; >> + 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)) >> + { >> + event_node = mxmlLoadString(NULL, CWMP_INFORM_EVENT_STRUCT, MXML_NO_CALLBACK); >> + if (!event_node) >> + return 0; >> + code_node = mxmlFindElement(event_node, event_node, "EventCode", NULL, NULL, MXML_DESCEND); >> + if (!code_node) >> + return 0; >> + code_node = mxmlNewText(code_node, 0, c); >> + if (!code_node) >> + return 0; >> + mxmlAdd (node,MXML_ADD_AFTER,MXML_ADD_TO_PARENT,event_node); >> + n++; >> + } >> + >> + if (n) >> + { >> + sprintf (buf,"cwmp:EventStruct[%u]",n); >> + mxmlElementSetAttr(node, "soap_enc:arrayType", buf ); /* I - Attribute value */ >> + } >> + >> + return 1; >> +} >> + >> +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)) >> goto error; >> >> busy_node = mxmlFindElement(tree, tree, "CurrentTime", NULL, NULL, MXML_DESCEND); > That would be it for now. I'll try and merge your patch this week. > > I would also like to invite you to #freecwmp IRC channel on freenode. > > Regards, > Luka > Regards Mohamed --------------070001010803020707080504 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit
Hi
It's pleasure to me contribute to your project and thank you for accepting the path
Please , see here after my response to your comments.

Le 28/08/2012 13:28, Luka Perkov a écrit :
Hi Mohamed,

On Tue, Aug 21, 2012 at 09:11:00AM +0100, KALLEL Mohamed wrote:

I'm Mohamed KALLEL and I'm interested in your freecwmp project.

Welcome aboard!


And I want to contribute in the development with the following patch
(patch attached with this email).

Thank you. I'll try to merge it this week, for now I'll give some quick
comments bellow.


The patch allow to support many events in the inform message. the
old behavior support only one event in the inform message
this patch allow to add the boot event (with the bootstarp event) in
the inform message.
the patch is based on the revision: *6d074ab*

Mohamed KALLEL


diff --git a/bin/Makefile.am b/bin/Makefile.am
index aa4db87..d7abfe2 100644
--- a/bin/Makefile.am
+++ b/bin/Makefile.am
@@ -7,6 +7,8 @@ freecwmpd_SOURCES =		\
 	../src/config.c		\
 	../src/cwmp/cwmp.h	\
 	../src/cwmp/cwmp.c	\
+	../src/cwmp/event.h	\
+	../src/cwmp/event.c	\
 	../src/cwmp/external.h	\
 	../src/cwmp/external.c	\
 	../src/http/b64.h	\
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

I think we could leave this out and by default if there is no event list
we use this two options (bootstrap and boot).
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


 config acs
 	option scheme http
diff --git a/src/config.c b/src/config.c
index ad7a86e..abc621c 100644
--- a/src/config.c
+++ b/src/config.c
@@ -12,6 +12,7 @@
 
 #include "config.h"
 #include "cwmp/cwmp.h"
+#include "cwmp/event.h"
 
 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;
 
 	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)
+			{
+				if((ie != NULL)&&(ie->name!=NULL))
+				{
+					if (!strcasecmp("bootstrap", ie->name))
+						event_add_event(BOOTSTRAP,NULL);
 
-			if (!strcasecmp("boot", uci_to_option(e)->v.string))
-				config->local->event = BOOT;
+					if (!strcasecmp("boot", ie->name))
+						event_add_event(BOOT,NULL);
 
-			DD("freecwmp.@local[0].event=%s\n", uci_to_option(e)->v.string);
+					DD("freecwmp.@local[0].event=%s\n", ie->name);

I'm not sure if this output will be the same as the one from uci. I'll
double check.
I Know that the output is not the same as the one from uci. but this the quick solution I found for this kind of debug message. (It's just a debug message, so that why I did not look for a solution this)
 


+				}
+			}
 		}
 
 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);
 	}
 
 	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);
 	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);
 	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);
 		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)
+	{

Coding style tip:

	if (order == FIRST || ilist == NULL) {

That goes for all your if's and for's...


+		ilist = cwmp.event_list;
+	}
+	ilist = ilist->next;
+	if (ilist != cwmp.event_list)
+	{
+		event = list_entry (ilist,struct event,list);
+		switch (event->code) {
 		case BOOT:
 			cwmp_inform_event_code = "1 BOOT";
 			break;
@@ -682,6 +694,7 @@ cwmp_get_event_code(void)
 		default:
 			cwmp_inform_event_code = "0 BOOTSTRAP";
 			break;
+		}
 	}
 
 	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);
 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

GPLv2 license header is missing...

Also I'll see if this can go in some existing file.
I suggest to keep both file untill we finish the event behaviours . Because, as indicated in the standard, the management of the event 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, 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...) 


@@ -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)

Please name your variables like this command_key instead commandKey.


+{
+	struct event *event;
+
+	event = calloc(1,sizeof(struct event));
+	event->code = code;
+	event->commandKey = commandKey;
+	list_add (&(event->list), &(event_list));
+
+	FC_DEVEL_DEBUG("exit");
+}
+
+void
+event_remove_all_events()
+{
+	struct event *event;
+
+	FC_DEVEL_DEBUG("enter");

I'm doing some cleanup so I'll get rid of FC_DEVEL_DEBUG in the near
future. It's ok for now, I just wanted to give you heads up.


+
+	while (event_list.next != &event_list)
+	{
+		event = list_entry (event_list.next,struct event, list);
+		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 @@

GPLv2 license header is missing...


+#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];
+	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))
+	{
+		event_node	= mxmlLoadString(NULL, CWMP_INFORM_EVENT_STRUCT, MXML_NO_CALLBACK);
+		if (!event_node)
+			return 0;
+		code_node	= mxmlFindElement(event_node, event_node, "EventCode", NULL, NULL, MXML_DESCEND);
+		if (!code_node)
+			return 0;
+		code_node	= mxmlNewText(code_node, 0, c);
+		if (!code_node)
+			return 0;
+		mxmlAdd (node,MXML_ADD_AFTER,MXML_ADD_TO_PARENT,event_node);
+		n++;
+	}
+
+	if (n)
+	{
+		sprintf (buf,"cwmp:EventStruct[%u]",n);
+		mxmlElementSetAttr(node, "soap_enc:arrayType", buf );	/* I - Attribute value */
+	}
+
+	return 1;
+}
+
+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))
 		goto error;
 
 	busy_node = mxmlFindElement(tree, tree, "CurrentTime", NULL, NULL, MXML_DESCEND);

That would be it for now. I'll try and merge your patch this week.

I would also like to invite you to #freecwmp IRC channel on freenode.

Regards,
Luka
Regards
Mohamed
--------------070001010803020707080504-- From mohamed.kallel@pivasoftware.com Wed Aug 29 10:09:16 2012 Received: with ECARTIS (v1.0.0; list freecwmp); Wed, 29 Aug 2012 10:09:26 +0200 (CEST) Received: from moutng.kundenserver.de ([212.227.17.10]:63964 "EHLO moutng.kundenserver.de" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S1902755Ab2H2IJQ (ORCPT ); Wed, 29 Aug 2012 10:09:16 +0200 Received: from [127.0.0.1] ([41.224.250.29]) by mrelayeu.kundenserver.de (node=mreu1) with ESMTP (Nemesis) id 0MPt6U-1T2nZl0jkw-004V32; Wed, 29 Aug 2012 10:09:05 +0200 Message-ID: <503DCE1A.6070605@pivasoftware.com> Date: Wed, 29 Aug 2012 09:08:58 +0100 From: KALLEL Mohamed User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20120713 Thunderbird/14.0 MIME-Version: 1.0 To: Jonas Gorski CC: freecwmp@linux-mips.org, John Crispin Subject: Re: patch for the freecwmp project References: <50334294.9020508@pivasoftware.com> <50334314.7020807@phrozen.org> In-Reply-To: Content-Type: multipart/alternative; boundary="------------080301080903030205020705" X-Provags-ID: V02:K0:roP3U4FtA0Kt8cMNtoJqqvZgOoGXssbbcss3/EybMlW 9LHhwt2fpJRpSOfR/JpWja6vlkr7xJhV1GPJdmkxVtlDjQdFSh sA2aBfVupjyX8be6X3BrxN8yKIrhHc9Ma3fqhSmD0H+XjNHgM7 hb96dhymf9T2CYzmccQpBML6U/os/Mii8a9HxkrwHZwXq3gB+E v3MFoNFfS8BadEn20ZXAKU3yW7oND5o1ZHcw/E8zGzIQVD3crZ cnpWNGdAZ1afJgAwMgTcvQED5YaR/NEO7R3SLCxm0nVvvmx8Ei IIn+L4qywRZWF8/auYJy+FEQfv2tBybYI3Ob/kyC97Y/LNdirh Wxj6qfpjedBFPY+NNC1H8vcV9W2X6dmoLe8xopUiv Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s freecwmp"> (uid 0) X-Orcpt: rfc822;freecwmp@linux-mips.org Original-Recipient: rfc822;freecwmp@linux-mips.org X-archive-position: 56 X-ecartis-version: Ecartis v1.0.0 Sender: freecwmp-bounce@linux-mips.org Errors-to: freecwmp-bounce@linux-mips.org X-original-sender: mohamed.kallel@pivasoftware.com Precedence: bulk X-list: freecwmp This is a multi-part message in MIME format. --------------080301080903030205020705 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 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 >> >> #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 >> +#include >> + >> +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 @@ >> "" \ >> "" \ >> "" \ >> - "" \ >> - "" \ >> - "" \ >> - "" \ >> - "" \ >> + "" \ >> "" \ >> "1" \ >> "" \ >> @@ -88,6 +84,11 @@ >> "" \ >> "" >> >> +#define CWMP_INFORM_EVENT_STRUCT \ >> +"" \ >> + "" \ >> + "" \ >> +"" >> >> #define CWMP_RESPONSE_MESSAGE \ >> "" \ >> 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 --------------080301080903030205020705 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit
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

  


--------------080301080903030205020705--

From freecwmp@lukaperkov.net Wed Aug 29 22:05:06 2012
Received: with ECARTIS (v1.0.0; list freecwmp); Wed, 29 Aug 2012 22:05:10 +0200 (CEST)
Received: from mail-wg0-f43.google.com ([74.125.82.43]:63278 "EHLO
        mail-wg0-f43.google.com" rhost-flags-OK-OK-OK-OK)
        by eddie.linux-mips.org with ESMTP id S1903251Ab2H2UFG (ORCPT
        ); Wed, 29 Aug 2012 22:05:06 +0200
Received: by wgbdr1 with SMTP id dr1so673922wgb.24
        for ; Wed, 29 Aug 2012 13:05:00 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=google.com; s=20120113;
        h=date:from:to:cc:subject:message-id:mail-followup-to:references
         :mime-version:content-type:content-disposition:in-reply-to
         :user-agent:x-gm-message-state;
        bh=AiPCRpeYCn7M49Tx2knIDSP6r7mqx9yl5YK23MiUvdA=;
        b=UBv95Ea/2g75iTZ87N8QAonVrDLQyw5kru2EEON8oDANB8MCoeZsxzoXjn2K0i2+C9
         7bjwhZpb/cHi0KfLzafTAGtX/zk+U/9FuELEA6L4ioQOFTUlJUpbBhnak4dDMWQmk0Zn
         jABvF3sUl4YkqQ82rRnEirollBdocrnh9o3xLbXBJQlwC0qDj3UD3gW/nXY2gzonvF4G
         yRKkzIAQMsfzOJheJgaNaaG4ttl4O2Fl4kLioHTjqRfTfV6mLt3r1wiJu8AzRkg2r6hr
         iTBTsgA0JKm7pBB9Px6Srut3ocxx1IxY/uyFr8jnVR0GBHMlFoRooJGoB/7yvkLYcp6X
         HIKw==
Received: by 10.180.81.165 with SMTP id b5mr42563963wiy.17.1346270700562;
        Wed, 29 Aug 2012 13:05:00 -0700 (PDT)
Received: from localhost (213-191-157-42.dhcp.iskon.hr. [213.191.157.42])
        by mx.google.com with ESMTPS id k20sm11803642wiv.11.2012.08.29.13.04.53
        (version=TLSv1/SSLv3 cipher=OTHER);
        Wed, 29 Aug 2012 13:04:57 -0700 (PDT)
Date:   Wed, 29 Aug 2012 22:05:06 +0200
From:   Luka Perkov 
To:     KALLEL Mohamed 
Cc:     freecwmp@linux-mips.org
Subject: Re: patch for the freecwmp project
Message-ID: <20120829200506.GA16080@w500.lan>
Mail-Followup-To: KALLEL Mohamed ,
        freecwmp@linux-mips.org
References: <50334294.9020508@pivasoftware.com>
 <50334314.7020807@phrozen.org>
 
 <503DCE1A.6070605@pivasoftware.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <503DCE1A.6070605@pivasoftware.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
X-Gm-Message-State: ALoCoQlm/o3qEe/eLoc+P+mnb/z9pMd4nLDD1DMIoC4+Jugnfcb0wAoI/GgvuF6qk/I19GUqe3XV
Return-Path: 
X-Envelope-To: <"|/home/ecartis/ecartis -s freecwmp"> (uid 0)
X-Orcpt: rfc822;freecwmp@linux-mips.org
Original-Recipient: rfc822;freecwmp@linux-mips.org
X-archive-position: 57
X-ecartis-version: Ecartis v1.0.0
Sender: freecwmp-bounce@linux-mips.org
Errors-to: freecwmp-bounce@linux-mips.org
X-original-sender: freecwmp@lukaperkov.net
Precedence: bulk
X-list: freecwmp

Hi Mohamed,

On Wed, Aug 29, 2012 at 09:08:58AM +0100, KALLEL Mohamed wrote:
> >>--- 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

I dont think this is a good idea. I dont want freecwmp starter script to
touch the freecwmp config. This needs to be handled in freecwmp core.

> >>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 agree with Jonas here.

> >>+	return 1;
> >You should return FC_SUCCESS here (which is 0). Also you are missing
> >FC_DEVEL_DEBUG("enter/exit") calls.

I will remove all FC_DEVEL_DEBUG("enter/exit") calls soon so it's ok if
they are missing.

Mohamed please resend your patch with the fixes Jonas and me pointed
out. After that we'll review it once again and see if it's ok for
merging.

Regards,
Luka