freecwmp
[Top] [All Lists]

Re: patch for the freecwmp project

To: KALLEL Mohamed <mohamed.kallel@pivasoftware.com>
Subject: Re: patch for the freecwmp project
From: Luka Perkov <freecwmp@lukaperkov.net>
Date: Tue, 28 Aug 2012 14:28:41 +0200
Cc: freecwmp@linux-mips.org
In-reply-to: <50334294.9020508@pivasoftware.com>
Mail-followup-to: KALLEL Mohamed <mohamed.kallel@pivasoftware.com>, freecwmp@linux-mips.org
Original-recipient: rfc822;freecwmp@linux-mips.org
References: <50334294.9020508@pivasoftware.com>
Sender: freecwmp-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
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

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