freecwmp
[Top] [All Lists]

Re: [PATCH 02/27] get_parameter values handler from xml is supporting ma

To: mohamed.kallel@pivasoftware.com
Subject: Re: [PATCH 02/27] get_parameter values handler from xml is supporting many parameters from external ubus output Contributed by Inteno Broadband Technology AB && PIVA SOFTWARE
From: Luka Perkov <freecwmp@lukaperkov.net>
Date: Sat, 8 Dec 2012 11:30:11 +0100
Cc: freecwmp@linux-mips.org, ahmed.zribi@pivasoftware.com, freecwmp@lukaperkov.net, jogo@openwrt.org
In-reply-to: <1354809292-2467-3-git-send-email-mohamed.kallel@pivasoftware.com>
Mail-followup-to: mohamed.kallel@pivasoftware.com, freecwmp@linux-mips.org, ahmed.zribi@pivasoftware.com, jogo@openwrt.org
Original-recipient: rfc822;freecwmp@linux-mips.org
References: <mohamed.kallel@pivasoftware.com> <1354809292-2467-1-git-send-email-mohamed.kallel@pivasoftware.com> <1354809292-2467-3-git-send-email-mohamed.kallel@pivasoftware.com>
Sender: freecwmp-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
Missing commit message. Contributed by must go in commit message and not
in subject.

On Thu, Dec 06, 2012 at 04:54:27PM +0100, Mohamed wrote:
> 
> Signed-off-by: Ahmed ZRIBI <ahmed.zribi@pivasoftware.com>
> Signed-off-by: Mohamed <mohamed.kallel@pivasoftware.com>
> ---
>  bin/Makefile.am |    3 +-
>  configure.ac    |    3 ++
>  src/external.c  |   95 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/external.h  |    6 +++-
>  src/freecwmp.c  |   17 ++++++++++
>  src/xml.c       |   66 +++++++++++++++++++++++--------------
>  6 files changed, 163 insertions(+), 27 deletions(-)
> 
> diff --git a/bin/Makefile.am b/bin/Makefile.am
> index 78cb111..ac96905 100644
> --- a/bin/Makefile.am
> +++ b/bin/Makefile.am
> @@ -48,5 +48,6 @@ freecwmpd_LDADD =           \
>       $(LIBUBUS_LIBS)         \
>       $(MICROXML_LIBS)        \
>       $(LIBCURL_LIBS)         \
> -     $(LIBZSTREAM_LIBS)
> +     $(LIBZSTREAM_LIBS)      \
> +     $(LIBPTHREAD_LIBS)
>  
> diff --git a/configure.ac b/configure.ac
> index c646c3a..38d3df4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -80,6 +80,9 @@ AC_SUBST([LIBUBUS_LDFLAGS])
>  LIBUBUS_LIBS='-lubus'
>  AC_SUBST([LIBUBUS_LIBS])
>  
> +LIBPTHREAD_LIBS='-lpthread'
> +AC_SUBST([LIBPTHREAD_LIBS])
> +

This should go after line 30 and not here:

 30 # checks for libraries

>  AM_COND_IF([HTTP_CURL], [
>   AC_DEFINE(HTTP_CURL)
>   PKG_CHECK_MODULES(LIBCURL, [libcurl])
> diff --git a/src/external.c b/src/external.c
> index c9c768f..94ae9b8 100644
> --- a/src/external.c
> +++ b/src/external.c
> @@ -25,8 +25,15 @@
>  #include "freecwmp.h"
>  
>  static struct uloop_process uproc;
> +pthread_t ubus_thread;
>  LIST_HEAD(external_list_parameter);
>  
> +void *thread_uloop_run (void *v)

No space here "thread_uloop_run(".

> +{
> +     uloop_run();
> +     return NULL;
> +}
> +
>  void external_add_list_paramameter(char *param_name, char *param_data, char 
> *param_type, char *fault_code)
>  {
>       struct external_parameter *external_parameter;
> @@ -39,6 +46,20 @@ void external_add_list_paramameter(char *param_name, char 
> *param_data, char *par
>       if (fault_code) external_parameter->fault_code = strdup(fault_code);
>  }
>  
> +void external_free_list_parameter()
> +{
> +     struct external_parameter *external_parameter;
> +     while (external_list_parameter.next!=&external_list_parameter) {

Missing space "next != &external".

> +             external_parameter = list_entry(external_list_parameter.next, 
> struct external_parameter, list);
> +             list_del(&external_parameter->list);
> +             free(external_parameter->name);
> +             free(external_parameter->data);
> +             free(external_parameter->type);
> +             free(external_parameter->fault_code);
> +             free(external_parameter);
> +     }
> +}
> +
>  int external_get_action(char *action, char *name, char **value)
>  {
>       freecwmp_log_message(NAME, L_NOTICE,
> @@ -121,6 +142,80 @@ error:
>       return -1;
>  }
>  
> +int external_get_action_write(char *action, char *name, char *arg)
> +{
> +     freecwmp_log_message(NAME, L_NOTICE,
> +             "adding to get %s script '%s'\n", action, name);
> +
> +     FILE *fp;
> +
> +     if (access(fc_script_get_actions, R_OK | W_OK | X_OK) != -1) {
> +             fp = fopen(fc_script_get_actions, "a");
> +             if (!fp) return -1;
> +     } else {
> +             fp = fopen(fc_script_get_actions, "w");
> +             if (!fp) return -1;
> +
> +             fprintf(fp, "#!/bin/sh\n");
> +
> +             if (chmod(fc_script_get_actions,
> +                     strtol("0700", 0, 8)) < 0) {
> +                     return -1;
> +     }

Wrong indentation.

> +     }
> +
> +#ifdef DUMMY_MODE
> +     fprintf(fp, "/bin/sh `pwd`/%s get %s %s %s\n", fc_script, action, name, 
> arg?arg:"");
> +#else
> +     fprintf(fp, "/bin/sh %s get %s %s %s\n", fc_script, action, name, 
> arg?arg:"");
> +#endif

Reformat like in the existing code:

#ifdef DUMMY_MODE
        fprintf(fp, "/bin/sh `pwd`/%s get %s %s '%s'\n", fc_script, action, 
name, arg ? arg : "");
#else
        fprintf(fp, "/bin/sh %s get %s %s '%s'\n", fc_script, action, name, arg 
? arg : "");
#endif

> +
> +     fclose(fp);
> +
> +     return 0;
> +}
> +
> +int external_get_action_execute()
> +{
> +     freecwmp_log_message(NAME, L_NOTICE, "executing get script\n");
> +
> +     pthread_create(&ubus_thread, NULL, &thread_uloop_run, NULL);
> +
> +     if ((uproc.pid = fork()) == -1) {
> +             return -1;
> +     }
> +
> +     if (uproc.pid == 0) {
> +             /* child */
> +
> +             const char *argv[3];
> +             int i = 0;
> +             argv[i++] = "/bin/sh";
> +             argv[i++] = fc_script_get_actions;
> +             argv[i++] = NULL;
> +
> +             execvp(argv[0], (char **) argv);
> +             exit(ESRCH);
> +
> +     } else if (uproc.pid < 0)
> +     return -1;
> +
> +     /* parent */
> +     int status;
> +     while (wait(&status) != uproc.pid) {
> +             DD("waiting for child to exit");
> +     }
> +
> +     pthread_cancel(ubus_thread);
> +     pthread_join(ubus_thread,NULL);

Missing space.

> +
> +     // TODO: add some kind of checks
> +
> +     remove(fc_script_get_actions);
> +
> +     return 0;
> +}
> +
>  int external_set_action_write(char *action, char *name, char *value)
>  {
>       freecwmp_log_message(NAME, L_NOTICE,
> diff --git a/src/external.h b/src/external.h
> index 64b4c6b..c6df2ea 100644
> --- a/src/external.h
> +++ b/src/external.h
> @@ -16,7 +16,8 @@ static char *fc_script = 
> "./ext/openwrt/scripts/freecwmp.sh";
>  #else
>  static char *fc_script = "/usr/sbin/freecwmp";
>  #endif
> -static char *fc_script_set_actions = "/tmp/freecwmp_set_action_values.sh";
> +static char *fc_script_set_actions = "/tmp/freecwmp_set_action.sh";
> +static char *fc_script_get_actions = "/tmp/freecwmp_get_action.sh";

Do we really need both? Why not use just "/tmp/freecwmp_actions.sh"?

>  
>  struct external_parameter {
>       struct list_head list;
> @@ -27,11 +28,14 @@ struct external_parameter {
>  };
>  
>  int external_get_action(char *action, char *name, char **value);
> +int external_get_action_write(char *action, char *name, char *arg);
> +int external_get_action_execute();
>  int external_set_action_write(char *action, char *name, char *value);
>  int external_set_action_execute();
>  int external_simple(char *arg);
>  int external_download(char *url, char *size);
>  void external_add_list_paramameter(char *param_name, char *param_data, char 
> *param_type, char *fault_code);
> +void external_free_list_parameter();
>  
>  #endif
>  
> diff --git a/src/freecwmp.c b/src/freecwmp.c
> index 4ef8395..f4c9cb1 100644
> --- a/src/freecwmp.c
> +++ b/src/freecwmp.c
> @@ -18,6 +18,7 @@
>  #include <arpa/inet.h>
>  #include <linux/netlink.h>
>  #include <linux/rtnetlink.h>
> +#include <signal.h>
>  
>  #include <libfreecwmp.h>
>  #include <libubox/uloop.h>
> @@ -194,11 +195,27 @@ netlink_init(void)
>       return 0;
>  }
>  
> +void signal_kill_all_handler(int sig)
> +{
> +     pthread_exit(NULL);

Use tabs and not spaces.

> +}
> +
> +
> +

Too many new lines.

>  int main (int argc, char **argv)
>  {
>       freecwmp_log_message(NAME, L_NOTICE, "daemon started\n");
>  
>       bool foreground = false;
> +     struct sigaction sigint_action;
> +
> +     sigint_action.sa_handler = &signal_kill_all_handler;
> +     sigemptyset (&sigint_action.sa_mask);

No space here.

> +     /* reset handler in case when pthread_cancel didn't stop
> +        threads for some reason */

Use u-boot coding style for multiline comments:

        /*
         * reset handler in case when pthread_cancel didn't stop
         * threads for some reason
         */

> +     sigint_action.sa_flags = SA_RESETHAND;
> +     sigaction(SIGTERM, &sigint_action, NULL);
> +

Too many new lines.

>  
>       setlocale(LC_CTYPE, "");
>       umask(0037);
> diff --git a/src/xml.c b/src/xml.c
> index 9fa036b..23b9ec7 100644
> --- a/src/xml.c
> +++ b/src/xml.c
> @@ -21,6 +21,8 @@
>  #include "messages.h"
>  #include "time.h"
>  
> +extern struct list_head external_list_parameter;
> +
>  struct rpc_method {
>       const char *name;
>       int (*handler)(mxml_node_t *body_in, mxml_node_t *tree_in,
> @@ -628,10 +630,11 @@ int xml_handle_get_parameter_values(mxml_node_t 
> *body_in,
>                                   mxml_node_t *tree_in,
>                                   mxml_node_t *tree_out)
>  {
> -     mxml_node_t *n, *b = body_in;
> +     mxml_node_t *n, *parameter_list, *b = body_in;
> +     struct external_parameter *external_parameter;
>       char *parameter_name = NULL;
>       char *parameter_value = NULL;
> -     char *c;
> +     char *c=NULL;

Missing space. And we don't need to set it to NULL.

>       int counter = 0;
>  
>       n = mxmlFindElement(tree_out, tree_out, "soap_env:Body",
> @@ -655,29 +658,47 @@ int xml_handle_get_parameter_values(mxml_node_t 
> *body_in,
>                   !strcmp(b->parent->value.element.name, "string")) {
>                       parameter_name = b->value.text.string;
>               }
> -

Don't remove newline.

> +             if (b && b->type == MXML_ELEMENT && /* added in order to 
> support GetParameterValues with empty string*/

Use multiline comment above if.

> +                     !strcmp(b->value.element.name, "string") &&
> +                     !b->child) {
> +                     parameter_name = "";

Set it to NULL.

> +             }

Add newline.

>               if (parameter_name) {
>                       if (!config_get_cwmp(parameter_name, &parameter_value)) 
> {
> +                             
> external_add_list_paramameter(parameter_name,parameter_value,"xsd:string",NULL);

Missing spaces.

> +                             FREE(parameter_value);
>                               // got the parameter value using libuci
> -                     } else if (!external_get_action("value",
> -                                     parameter_name, &parameter_value)) {
> +                     } else if 
> (!external_get_action_write("value",parameter_name, NULL)) {

Missing spaces.

>                               // got the parameter value via external script
>                       } else {
>                               // error occurred when getting parameter value
>                               goto out;
>                       }
> -                     counter++;
> +             }
> +             b = mxmlWalkNext(b, body_in, MXML_DESCEND);
> +             parameter_name = NULL;
> +     }
>  
> -                     n = mxmlFindElement(tree_out, tree_out, 
> "ParameterList", NULL, NULL, MXML_DESCEND);
> -                     if (!n) goto out;
> +     if (external_get_action_execute())
> +             goto out;
> +
> +     parameter_list = mxmlFindElement(tree_out, tree_out, "ParameterList", 
> NULL, NULL, MXML_DESCEND);
> +     if (!parameter_list) goto out;
> +
> +     while (external_list_parameter.next!=&external_list_parameter) {

Missing spaces.

> +
> +             external_parameter = list_entry(external_list_parameter.next, 
> struct external_parameter, list);
>  
> -                     n = mxmlNewElement(n, "ParameterValueStruct");
> +             if (external_parameter->fault_code && 
> external_parameter->fault_code[0]=='9')
> +                     goto out; // KMD TODO return FAULT message the fault 
> code is in ->fault_code

Use braces and multiline comment. Make comment more readable.

> +
> +             n = mxmlNewElement(parameter_list, "ParameterValueStruct");
>                       if (!n) goto out;
>  
>                       n = mxmlNewElement(n, "Name");
>                       if (!n) goto out;
>  
> -                     n = mxmlNewText(n, 0, parameter_name);
> +             n = mxmlNewText(n, 0, external_parameter->name);

Wrong indentation.

>                       if (!n) goto out;
>  
>                       n = n->parent->parent;
> @@ -685,23 +706,19 @@ int xml_handle_get_parameter_values(mxml_node_t 
> *body_in,
>                       if (!n) goto out;
>  
>  #ifdef ACS_MULTI
> -                     mxmlElementSetAttr(n, "xsi:type", "xsd:string");
> +             mxmlElementSetAttr(n, "xsi:type", external_parameter->type);

Wrong indentation.

>  #endif
> -                     n = mxmlNewText(n, 0, parameter_value ? parameter_value 
> : "");
> +             n = mxmlNewText(n, 0, external_parameter->data? 
> external_parameter->data : "");

Wrong indentation. Missing spaces.

>                       if (!n) goto out;
>  
> -                     /*
> -                      * three day's work to finally find memory leak if we
> -                      * free parameter_name;
> -                      * it points to: b->value.text.string
> -                      *
> -                      * also, parameter_value can be NULL so we don't do 
> checks
> -                      */
> -                     parameter_name = NULL;
> -             }
> +             counter++;
>               
> -             FREE(parameter_value);
> -             b = mxmlWalkNext(b, body_in, MXML_DESCEND);
> +             list_del(&external_parameter->list);
> +             free(external_parameter->name);
> +             free(external_parameter->data);
> +             free(external_parameter->type);
> +             free(external_parameter->fault_code);
> +             free(external_parameter);
>       }
>  
>  #ifdef ACS_MULTI
> @@ -716,11 +733,10 @@ int xml_handle_get_parameter_values(mxml_node_t 
> *body_in,
>       FREE(c);
>  #endif
>  
> -     FREE(parameter_value);
>       return 0;
>  
>  out:
> -     FREE(parameter_value);
> +     external_free_list_parameter();
>       return -1;
>  }
>  
> -- 
> 1.7.4.1

Luka

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