freecwmp
[Top] [All Lists]

Re: NEW PATCH for data model

To: KALLEL Mohamed <mohamed.kallel@pivasoftware.com>
Subject: Re: NEW PATCH for data model
From: Luka Perkov <freecwmp@lukaperkov.net>
Date: Tue, 20 Nov 2012 11:25:08 +0100
Cc: Jonas Gorski <jonas.gorski@gmail.com>, freecwmp@linux-mips.org
In-reply-to: <50AA5B24.305@pivasoftware.com>
Mail-followup-to: KALLEL Mohamed <mohamed.kallel@pivasoftware.com>, Jonas Gorski <jonas.gorski@gmail.com>, freecwmp@linux-mips.org
Original-recipient: rfc822;freecwmp@linux-mips.org
References: <508E5D2B.6050508@pivasoftware.com> <50A676F4.8080708@pivasoftware.com> <CAOiHx==qi9w3a3Rj6JyLY9Np+uju=LR=AUi9OqhGTj-=6bG0bQ@mail.gmail.com> <50AA5B24.305@pivasoftware.com>
Sender: freecwmp-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
Hi Mohamed,

On Mon, Nov 19, 2012 at 05:15:32PM +0100, KALLEL Mohamed wrote:
> Please, Find attached the new patch file.

Please send patches inline and not attached. Also give us more
information about this patch.

It's hard to review all the changes you made. Please look how to
properly introduce big changes:

https://github.com/KanjiMonster/freecwmp/commits/rpc_rework

Look at commits made including and after "add generic rpc method
handling".

> It contains only support  communication from script to core with C
> 
> Signed-off-by: Mohamed <mohamed.kallel@pivasoftware.com>
> ---
>  configure.ac   |    3 +
>  src/external.c |  167 
> +++++++++++++++++++++++++++++++++++++++++++-------------
>  src/external.h |   16 +++++
>  src/freecwmp.c |   19 ++++++
>  src/freecwmp.h |    1 +
>  src/ubus.c     |  127 ++++++++++++++++++++++++++++++++++++++++++
>  src/xml.c      |  105 +++++++++++++++++++----------------
>  7 files changed, 353 insertions(+), 85 deletions(-)
> 
> 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])
> +

Why are you using threads and not uloop ?

>  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 9ac7b36..2222646 100644
> --- a/src/external.c
> +++ b/src/external.c
> @@ -25,15 +25,67 @@
>  #include "freecwmp.h"
>  
>  static struct uloop_process uproc;
> +pthread_t ubus_thread;
> +LIST_HEAD(external_list_parameter);
> +
> +void *thread_uloop_run (void *v)
> +{
> +     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;
> +     struct list_head *ilist; int i =0;
> +     external_parameter = calloc(1, sizeof(struct external_parameter));
> +     list_add_tail(&external_parameter->list,&external_list_parameter);
> +     if (param_name) external_parameter->name = strdup(param_name);
> +     if (param_data) external_parameter->data = strdup(param_data);
> +     if (param_type) external_parameter->type = strdup(param_type);
> +     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) {
> +             external_parameter = list_entry(external_list_parameter.next, 
> struct external_parameter, list);
> +             list_del(&external_parameter->list);
> +             CFREE(external_parameter->name);
> +             CFREE(external_parameter->data);
> +             CFREE(external_parameter->type);
> +             CFREE(external_parameter->fault_code);
> +             CFREE(external_parameter);
> +     }
> +}
> +
>  
>  int external_get_action(char *action, char *name, char **value)
>  {
> +     struct external_parameter *external_parameter;
> +     external_get_action_common(action, name, NULL);
> +     if (external_list_parameter.next!=&external_list_parameter) {
> +             external_parameter = list_entry(external_list_parameter.next, 
> struct external_parameter, list);
> +             if (external_parameter->data)
> +                     *value = external_parameter->data;
> +             list_del(&external_parameter->list);
> +             CFREE(external_parameter->name);
> +             CFREE(external_parameter->data);
> +             CFREE(external_parameter->type);
> +             CFREE(external_parameter->fault_code);
> +             CFREE(external_parameter);
> +     }
> +     external_free_list_parameter();
> +}
> +
> +int external_get_action_common(char *action, char *name, char *arg /* arg is 
> added for GetParameterNames NextLevel argument*/)
> +{
>       freecwmp_log_message(NAME, L_NOTICE,
>                            "executing get %s '%s'\n", action, name);
>  
> -     int pfds[2];
> -     if (pipe(pfds) < 0)
> -             return -1;
> +
> +     pthread_create(&ubus_thread, NULL, &thread_uloop_run, NULL);
>  
>       if ((uproc.pid = fork()) == -1)
>               goto error;
> @@ -45,67 +97,106 @@ int external_get_action(char *action, char *name, char 
> **value)
>               int i = 0;
>               argv[i++] = "/bin/sh";
>               argv[i++] = fc_script;
> -             argv[i++] = "--newline";
> -             argv[i++] = "--value";
>               argv[i++] = "get";
>               argv[i++] = action;
>               argv[i++] = name;
> +             if(arg) argv[i++] = arg;
>               argv[i++] = NULL;
>  
> -             close(pfds[0]);
> -             dup2(pfds[1], 1);
> -             close(pfds[1]);
> -
>               execvp(argv[0], (char **) argv);
>               exit(ESRCH);
>  
>       } else if (uproc.pid < 0)
>               goto error;
>  
> -     /* parent */
> -     close(pfds[1]);
> -
>       int status;
>       while (wait(&status) != uproc.pid) {
>               DD("waiting for child to exit");
>       }
> +     pthread_cancel(ubus_thread);
> +     pthread_join(ubus_thread,NULL);
>  
> -     char buffer[64];
> -     ssize_t rxed;
> -     char *c;
> -     int t;
> -
> -     *value = NULL;
> -     while ((rxed = read(pfds[0], buffer, sizeof(buffer))) > 0) {
> -             if (*value)
> -                     t = asprintf(&c, "%s%.*s", *value, (int) rxed, buffer);
> -             else
> -                     t = asprintf(&c, "%.*s", (int) rxed, buffer);
> +     return 0;
>  
> -             if (t == -1) goto error;
> +error:
> +     return -1;
>  
> -             free(*value);
> -             *value = strdup(c);
> -             free(c);
>       }
>  
> -     if (!strlen(*value)) {
> -             FREE(*value);
> -             goto done;
> +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;
> +             }
>       }
>  
> -     if (rxed < 0)
> -             goto error;
> +#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);
>  
> -done:
> -     close(pfds[0]);
>       return 0;
> +}
>  
> -error:
> -     FREE(*c);
> -     FREE(*value);
> -     close(pfds[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);
> +
> +
> +     // TODO: add some kind of checks
> +
> +     if (remove(fc_script_get_actions) != 0)
>       return -1;
> +
> +     return 0;
>  }
>  
>  int external_set_action_write(char *action, char *name, char *value)
> diff --git a/src/external.h b/src/external.h
> index 8606266..fcf9819 100644
> --- a/src/external.h
> +++ b/src/external.h
> @@ -9,6 +9,7 @@
>  
>  #ifndef _FREECWMP_EXTERNAL_H__
>  #define _FREECWMP_EXTERNAL_H__
> +#include <libubox/list.h>
>  
>  #ifdef DUMMY_MODE
>  static char *fc_script = "./ext/openwrt/scripts/freecwmp.sh";
> @@ -16,12 +17,27 @@ static char *fc_script = 
> "./ext/openwrt/scripts/freecwmp.sh";
>  static char *fc_script = "/usr/sbin/freecwmp";
>  #endif
>  static char *fc_script_set_actions = "/tmp/freecwmp_set_action_values.sh";
> +static char *fc_script_get_actions = "/tmp/freecwmp_get_action_values.sh";
> +
> +struct external_parameter {
> +     struct list_head list;
> +     char *name;
> +     char *data; /* notification for GetParameterAttribute; writable for 
> GetParameterNames; value for GetParameterValues*/
> +     char *type;
> +     char *fault_code;
> +};
> +

Remove additional newlines.

>  
>  int external_get_action(char *action, char *name, char **value);
> +int external_get_action_common(char *action, char *name, char *arg);
> +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..7808b43 100644
> --- a/src/freecwmp.c
> +++ b/src/freecwmp.c
> @@ -18,6 +18,9 @@
>  #include <arpa/inet.h>
>  #include <linux/netlink.h>
>  #include <linux/rtnetlink.h>
> +#include <signal.h>
> +
> +

Remove additional newlines.

>  
>  #include <libfreecwmp.h>
>  #include <libubox/uloop.h>
> @@ -194,11 +197,27 @@ netlink_init(void)
>       return 0;
>  }
>  
> +void signal_kill_all_handler(int sig)
> +{
> +     pthread_exit(NULL);
> +}
> +
> +
> +
>  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);
> +     /* 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);
> +
>  
>       setlocale(LC_CTYPE, "");
>       umask(0037);
> diff --git a/src/freecwmp.h b/src/freecwmp.h
> index cb2a548..22d22cf 100644
> --- a/src/freecwmp.h
> +++ b/src/freecwmp.h
> @@ -13,6 +13,7 @@
>  #define NAME "freecwmpd"
>  
>  #define FREE(x) do { free(x); x = NULL; } while (0);
> +#define CFREE(x) if (x) free(x);

Either use free() function or use FREE macro. free(NULL) is valid in C.

>  
>  #ifdef DEBUG
>  #define D(format, ...) fprintf(stderr, "%s(%d): " format, __func__, 
> __LINE__, ## __VA_ARGS__)
> diff --git a/src/ubus.c b/src/ubus.c
> index c10170d..b2e9681 100644
> --- a/src/ubus.c
> +++ b/src/ubus.c
> @@ -16,6 +16,7 @@
>  #include "config.h"
>  #include "cwmp.h"
>  #include "freecwmp.h"
> +#include "external.h"
>  
>  static struct ubus_context *ctx = NULL;
>  static struct ubus_object main_object;
> @@ -157,10 +158,136 @@ freecwmpd_handle_command(struct ubus_context *ctx, 
> struct ubus_object *obj,
>       return 0;
>  }
>  
> +static enum getParamValues {
> +     GETPARAMVALUES_PARAM,

Rename "GETPARAMVALUES_" to "GET_PARAM_VALUES_"

> +     GETPARAMVALUES_VALUE,
> +     GETPARAMVALUES_TYPE,
> +     GETPARAMVALUES_FAULT,
> +     __GETPARAMVALUES_MAX
> +};
> +
> +static const struct blobmsg_policy getParamValues_policy[] = {
> +     [GETPARAMVALUES_PARAM] = { .name = "parameter", .type = 
> BLOBMSG_TYPE_STRING },
> +     [GETPARAMVALUES_VALUE] = { .name = "value", .type = BLOBMSG_TYPE_STRING 
> },
> +     [GETPARAMVALUES_TYPE] = { .name = "type", .type = BLOBMSG_TYPE_STRING },
> +     [GETPARAMVALUES_FAULT] = { .name = "fault_code", .type = 
> BLOBMSG_TYPE_STRING },

Is there any reason why this is not an BLOBMSG_TYPE_INT* ?

> +};
> +
> +static int
> +freecwmpd_handle_getParamValues(struct ubus_context *ctx, struct ubus_object 
> *obj,

Functions and variables  should be named with all lowercase letters.
Where needed separate with "_".

> +                     struct ubus_request_data *req, const char *method,
> +                     struct blob_attr *msg)
> +{
> +     struct blob_attr *tb[__GETPARAMVALUES_MAX];
> +
> +     blobmsg_parse(getParamValues_policy, ARRAY_SIZE(getParamValues_policy), 
> tb,
> +                   blob_data(msg), blob_len(msg));
> +
> +     if (!tb[GETPARAMVALUES_PARAM])
> +             return UBUS_STATUS_INVALID_ARGUMENT;
> +
> +
> +     freecwmp_log_message(NAME, L_NOTICE,
> +                          "triggered ubus GetParameterValues parameter %s\n",
> +                          blobmsg_data(tb[GETPARAMVALUES_PARAM]));
> +
> +

Remove additional newlines.

> +     external_add_list_paramameter(blobmsg_data(tb[GETPARAMVALUES_PARAM]),
> +                     tb[GETPARAMVALUES_VALUE]? 
> blobmsg_data(tb[GETPARAMVALUES_VALUE]) : NULL,
> +                     tb[GETPARAMVALUES_TYPE]? 
> blobmsg_data(tb[GETPARAMVALUES_TYPE]) : "xsd:string",
> +                     tb[GETPARAMVALUES_FAULT]? 
> blobmsg_data(tb[GETPARAMVALUES_FAULT]) : NULL);
> +
> +     return 0;
> +}
> +
> +static enum getParamNames {
> +     GETPARAMNAMES_PARAM,

Rename "GETPARAMNAMES_" to "GET_PARAM_NAMES_".
> +     GETPARAMNAMES_WRITABLE,
> +     GETPARAMNAMES_FAULT,
> +     __GETPARAMNAMES_MAX
> +};
> +
> +static const struct blobmsg_policy getParamNames_policy[] = {
> +     [GETPARAMNAMES_PARAM] = { .name = "parameter", .type = 
> BLOBMSG_TYPE_STRING },
> +     [GETPARAMNAMES_WRITABLE] = { .name = "writable", .type = 
> BLOBMSG_TYPE_STRING },

Is there any reason why this is not BLOBMSG_TYPE_INT8 ?

> +     [GETPARAMNAMES_FAULT] = { .name = "fault_code", .type = 
> BLOBMSG_TYPE_STRING },

Is there any reason why this is not an BLOBMSG_TYPE_INT* ?

> +};
> +
> +static int
> +freecwmpd_handle_getParamNames(struct ubus_context *ctx, struct ubus_object 
> *obj,
> +                     struct ubus_request_data *req, const char *method,
> +                     struct blob_attr *msg)
> +{
> +     struct blob_attr *tb[__GETPARAMNAMES_MAX];
> +
> +     blobmsg_parse(getParamNames_policy, ARRAY_SIZE(getParamNames_policy), 
> tb,
> +                   blob_data(msg), blob_len(msg));
> +
> +     if (!tb[GETPARAMNAMES_PARAM])
> +             return UBUS_STATUS_INVALID_ARGUMENT;
> +
> +
> +     freecwmp_log_message(NAME, L_NOTICE,
> +                          "triggered ubus GetParameterNames parameter %s\n",
> +                          blobmsg_data(tb[GETPARAMNAMES_PARAM]));
> +
> +

Remove additional newlines.

> +     external_add_list_paramameter(blobmsg_data(tb[GETPARAMNAMES_PARAM]),
> +                     tb[GETPARAMNAMES_WRITABLE]? 
> blobmsg_data(tb[GETPARAMNAMES_WRITABLE]) : NULL,
> +                     NULL,
> +                     tb[GETPARAMNAMES_FAULT]? 
> blobmsg_data(tb[GETPARAMNAMES_FAULT]) : NULL);
> +
> +     return 0;
> +}
> +
> +static enum getParamAttributes {
> +     GETPARAMATTRIBUTES_PARAM,
> +     GETPARAMATTRIBUTES_NOTIF,
> +     GETPARAMATTRIBUTES_FAULT,
> +     __GETPARAMATTRIBUTES_MAX
> +};
> +
> +static const struct blobmsg_policy getParamAttributes_policy[] = {
> +     [GETPARAMATTRIBUTES_PARAM] = { .name = "parameter", .type = 
> BLOBMSG_TYPE_STRING },
> +     [GETPARAMATTRIBUTES_NOTIF] = { .name = "notification", .type = 
> BLOBMSG_TYPE_STRING },
> +     [GETPARAMATTRIBUTES_FAULT] = { .name = "fault_code", .type = 
> BLOBMSG_TYPE_STRING },
> +};
> +
> +static int
> +freecwmpd_handle_getParamAttributes(struct ubus_context *ctx, struct 
> ubus_object *obj,
> +                     struct ubus_request_data *req, const char *method,
> +                     struct blob_attr *msg)
> +{
> +     struct blob_attr *tb[__GETPARAMATTRIBUTES_MAX];
> +
> +     blobmsg_parse(getParamAttributes_policy, 
> ARRAY_SIZE(getParamAttributes_policy), tb,
> +                   blob_data(msg), blob_len(msg));
> +
> +     if (!tb[GETPARAMATTRIBUTES_PARAM])
> +             return UBUS_STATUS_INVALID_ARGUMENT;
> +
> +     if (!tb[GETPARAMATTRIBUTES_NOTIF])
> +             return UBUS_STATUS_INVALID_ARGUMENT;
> +
> +     freecwmp_log_message(NAME, L_NOTICE,
> +                          "triggered ubus GetParameterAttributes parameter 
> %s\n",
> +                          blobmsg_data(tb[GETPARAMATTRIBUTES_PARAM]));
> +
> +     
> external_add_list_paramameter(blobmsg_data(tb[GETPARAMATTRIBUTES_PARAM]),
> +                                   
> blobmsg_data(tb[GETPARAMATTRIBUTES_NOTIF]),
> +                                   NULL,
> +                                   
> blobmsg_data(tb[GETPARAMATTRIBUTES_FAULT]));
> +
> +     return 0;
> +}
> +
>  static const struct ubus_method freecwmp_methods[] = {
>       UBUS_METHOD("notify", freecwmpd_handle_notify, notify_policy),
>       UBUS_METHOD("inform", freecwmpd_handle_inform, inform_policy),
>       UBUS_METHOD("command", freecwmpd_handle_command, command_policy),
> +     UBUS_METHOD("GetParameterValues", freecwmpd_handle_getParamValues, 
> getParamValues_policy),

Please rename this to:

UBUS_METHOD("get_parameter_values", freecwmpd_handle_get_parameter_values, 
get_parameter_values_policy),

> +     UBUS_METHOD("GetParameterNames", freecwmpd_handle_getParamNames, 
> getParamNames_policy),

See above.

> +     UBUS_METHOD("GetParameterAttributes", 
> freecwmpd_handle_getParamAttributes, getParamAttributes_policy),

See above.

>  };
>  
>  static struct ubus_object_type main_object_type =
> diff --git a/src/xml.c b/src/xml.c
> index 9fa036b..053ee1a 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,
> @@ -217,6 +219,7 @@ int xml_prepare_inform_message(char **msg_out)
>  {
>       mxml_node_t *tree, *b;
>       char *c, *tmp;
> +     struct external_parameter *external_parameter;
>  
>  #ifdef DUMMY_MODE
>       FILE *fp;
> @@ -335,37 +338,33 @@ int xml_prepare_inform_message(char **msg_out)
>       if (mxmlGetType(b) != MXML_ELEMENT)
>               goto error;
>  
> -     tmp = 
> "InternetGatewayDevice.WANDevice.1.WANConnectionDevice.1.WANIPConnection.1.ExternalIPAddress";
> -     b = mxmlFindElementText(tree, tree, tmp, MXML_DESCEND);
> -     if (!b) goto error;
> -
> -     b = b->parent->next->next;
> -     if (mxmlGetType(b) != MXML_ELEMENT)
> +     
> external_get_action_write("value","InternetGatewayDevice.WANDevice.1.WANConnectionDevice.1.WANIPConnection.1.ExternalIPAddress",
>  NULL);

Missing space here: "value","

> +     
> external_get_action_write("value","InternetGatewayDevice.ManagementServer.ConnectionRequestURL",
>  NULL);

Missing space here: "value","

> +     if (external_get_action_execute())
>               goto error;
>  
> -     c = NULL;
> -     if (external_get_action("value", tmp, &c)) goto error;
> -     if (c) {
> -             b = mxmlNewText(b, 0, c);
> -             FREE(c);
> -             if (!b) goto error;
> -     }
> -
> -     tmp = "InternetGatewayDevice.ManagementServer.ConnectionRequestURL";
> -     b = mxmlFindElementText(tree, tree, tmp, MXML_DESCEND);
> -     if (!b) goto error;
> +     while (external_list_parameter.next!=&external_list_parameter)
> +     {

Bracket should go in the same line like while.

> +             external_parameter = list_entry(external_list_parameter.next, 
> struct external_parameter, list);
> +             b = mxmlFindElementText(tree, tree, external_parameter->name, 
> MXML_DESCEND);
> +             if (!b) continue;
>  
>       b = b->parent->next->next;
>       if (mxmlGetType(b) != MXML_ELEMENT)
>               goto error;
>  
> -     c = NULL;
> -     if (external_get_action("value", tmp, &c)) goto error;
> -     if (c) {
> -             b = mxmlNewText(b, 0, c);
> -             FREE(c);
> +             if (external_parameter->data) {
> +                     b = mxmlNewText(b, 0, external_parameter->data);
>               if (!b) goto error;
>       }
> +             list_del(&external_parameter->list);
> +             CFREE(external_parameter->name);
> +             CFREE(external_parameter->data);
> +             CFREE(external_parameter->type);
> +             CFREE(external_parameter->fault_code);
> +             CFREE(external_parameter);
> +     }
> +
>  
>       if (xml_prepare_notifications_inform(tree))
>               goto error;
> @@ -376,6 +375,7 @@ int xml_prepare_inform_message(char **msg_out)
>       return 0;
>  
>  error:
> +     external_free_list_parameter();
>       mxmlDelete(tree);
>       return -1;
>  }
> @@ -628,10 +628,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, *ParameterList, *b = body_in;

Variables must be lower case.

> +     struct external_parameter *external_parameter;
>       char *parameter_name = NULL;
>       char *parameter_value = NULL;
> -     char *c;
> +     char *c = NULL;
>       int counter = 0;
>  
>       n = mxmlFindElement(tree_out, tree_out, "soap_env:Body",
> @@ -655,29 +656,44 @@ int xml_handle_get_parameter_values(mxml_node_t 
> *body_in,
>                   !strcmp(b->parent->value.element.name, "string")) {
>                       parameter_name = b->value.text.string;
>               }
> -
> +             if (b && b->type == MXML_ELEMENT && /* added in order to 
> support GetParameterValues with empty string*/
> +                     !strcmp(b->value.element.name, "string") &&
> +                     !b->child) {
> +                     parameter_name = "";
> +             }
>               if (parameter_name) {
>                       if (!config_get_cwmp(parameter_name, &parameter_value)) 
> {
> +                             
> external_add_list_paramameter(parameter_name,parameter_value,"xsd:string",NULL);
> +                             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)) {
>                               // 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;
> +
> +     ParameterList = mxmlFindElement(tree_out, tree_out, "ParameterList", 
> NULL, NULL, MXML_DESCEND);
> +     if (!ParameterList) goto out;
> +
> +     while (external_list_parameter.next!=&external_list_parameter) {
> +
> +             external_parameter = list_entry(external_list_parameter.next, 
> struct external_parameter, list);
>  
> -                     n = mxmlNewElement(n, "ParameterValueStruct");
> +             n = mxmlNewElement(ParameterList, "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);
>                       if (!n) goto out;
>  
>                       n = n->parent->parent;
> @@ -685,23 +701,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);
>  #endif
> -                     n = mxmlNewText(n, 0, parameter_value ? parameter_value 
> : "");
> +             n = mxmlNewText(n, 0, external_parameter->data? 
> external_parameter->data : "");
>                       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);
> +             CFREE(external_parameter->name);
> +             CFREE(external_parameter->data);
> +             CFREE(external_parameter->type);
> +             CFREE(external_parameter->fault_code);
> +             CFREE(external_parameter);
>       }
>  
>  #ifdef ACS_MULTI
> @@ -716,11 +728,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;
>  }
>  

Luka

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