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