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: Jonas Gorski <jonas.gorski@gmail.com>
Date: Tue, 28 Aug 2012 15:49:55 +0200
Cc: freecwmp@linux-mips.org, John Crispin <john@phrozen.org>
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==
In-reply-to: <50334314.7020807@phrozen.org>
Original-recipient: rfc822;freecwmp@linux-mips.org
References: <50334294.9020508@pivasoftware.com> <50334314.7020807@phrozen.org>
Sender: freecwmp-bounce@linux-mips.org
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.

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

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

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