freecwmp
[Top] [All Lists]

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

To: freecwmp@linux-mips.org, ahmed.zribi@pivasoftware.com, jogo@openwrt.org
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: KALLEL Mohamed <mohamed.kallel@pivasoftware.com>
Date: Mon, 24 Dec 2012 15:54:08 +0100
In-reply-to: <20121208103011-10595@mutt-kz>
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> <20121208103011-10595@mutt-kz>
Sender: freecwmp-bounce@linux-mips.org
User-agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Thunderbird/17.0
Answers and remarks

Le 08/12/2012 11:30, Luka Perkov a écrit :
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
The arg should not be set under '%s', This is not set parameter it's get parameter.

+
+       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.
in the code we added we make difference between NULL an empty message"". For us it's not the same. empty means that we have to return all parameter in the data model

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