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, ¶meter_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, ¶meter_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
|