freecwmp
[Top] [All Lists]

Re: [PATCH 14/27] add xml message get_parameter_name handler. Contribute

To: freecwmp@linux-mips.org, ahmed.zribi@pivasoftware.com, jogo@openwrt.org
Subject: Re: [PATCH 14/27] add xml message get_parameter_name handler. Contributed by Inteno Broadband Technology AB & PIVA SOFTWARE
From: KALLEL Mohamed <mohamed.kallel@pivasoftware.com>
Date: Mon, 24 Dec 2012 18:20:36 +0100
In-reply-to: <20121208104716-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-15-git-send-email-mohamed.kallel@pivasoftware.com> <20121208104716-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
Le 08/12/2012 11:47, 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:39PM +0100, Mohamed wrote:
Signed-off-by: Ahmed ZRIBI <ahmed.zribi@pivasoftware.com>
Signed-off-by: Mohamed <mohamed.kallel@pivasoftware.com>
---
  src/cwmp.c     |    2 +-
  src/external.c |   70 +++++++++++++-----------------------
  src/external.h |    3 +-
  src/ubus.c     |   39 ++++++++++++++++++++
  src/xml.c      |  111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
  src/xml.h      |    4 ++
  6 files changed, 181 insertions(+), 48 deletions(-)

diff --git a/src/cwmp.c b/src/cwmp.c
index b75c762..7de19e3 100644
--- a/src/cwmp.c
+++ b/src/cwmp.c
@@ -248,7 +248,7 @@ void cwmp_clear_events(void)
  void cwmp_add_notification(char *parameter, char *value, char *type)
  {
        char *c = NULL;
-       external_get_action("notification", parameter, &c);
+       external_get_action_data("notification", parameter, &c);
        if (!c) return;
struct notification *n = NULL;
diff --git a/src/external.c b/src/external.c
index 94ae9b8..4973d1a 100644
--- a/src/external.c
+++ b/src/external.c
@@ -60,14 +60,32 @@ void external_free_list_parameter()
        }
  }
-int external_get_action(char *action, char *name, char **value)
+int external_get_action_data(char *action, char *name, char **value)
+{
+       struct external_parameter *external_parameter;
+       external_get_action(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);
+               free(external_parameter->name);
+               free(external_parameter->data);
+               free(external_parameter->type);
+               free(external_parameter->fault_code);
+               free(external_parameter);
+       }
+       external_free_list_parameter();
+       return 0;
+}
+
+int external_get_action(char *action, char *name, char *arg /* arg is added 
for GetParameterNames NextLevel argument*/)
Comments don't go here.

  {
        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;
@@ -79,66 +97,28 @@ 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;
Missing space. And put a comment here and not inside the function above.

                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);
Missing space.

- 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);
-
-               if (t == -1) goto error;
-
-               free(*value);
-               *value = strdup(c);
-               free(c);
-       }
-
-       if (!strlen(*value)) {
-               FREE(*value);
-               goto done;
-       }
-
-       if (rxed < 0)
-               goto error;
-
-done:
-       close(pfds[0]);
        return 0;
error:
-       FREE(*c);
-       FREE(*value);
-       close(pfds[0]);
If we don't do any cleaning up here there should entire error should be
removed.
The above element c, value, pfds are not exist any more in the function, there is nothing to cleanup here in this function

        return -1;
  }
diff --git a/src/external.h b/src/external.h
index c6df2ea..8e82c39 100644
--- a/src/external.h
+++ b/src/external.h
@@ -27,7 +27,8 @@ struct external_parameter {
        char *fault_code;
  };
-int external_get_action(char *action, char *name, char **value);
+int external_get_action(char *action, char *name, char *arg);
+int external_get_action_data(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);
diff --git a/src/ubus.c b/src/ubus.c
index 0fd1def..cbf94b0 100644
--- a/src/ubus.c
+++ b/src/ubus.c
@@ -201,11 +201,50 @@ freecwmpd_handle_get_param_values(struct ubus_context 
*ctx, struct ubus_object *
        return 0;
  }
+static enum get_param_names {
+       GET_PARAM_NAMES_PARAM,
+       GET_PARAM_NAMES_WRITABLE,
+       GET_PARAM_NAMES_FAULT,
+       __GET_PARAM_NAMES_MAX
+};
+
+static const struct blobmsg_policy get_param_names_policy[] = {
+       [GET_PARAM_NAMES_PARAM] = { .name = "parameter", .type = 
BLOBMSG_TYPE_STRING },
+       [GET_PARAM_NAMES_WRITABLE] = { .name = "writable", .type = 
BLOBMSG_TYPE_STRING },
I don't like this "writable" parameter as it is now. It should be
renamed to permissions or something like that. That way we could control
both read and write.

And why is this BLOBMSG_TYPE_STRING?
The writable is the keyword used in the standard of TR069 So I prefer to keep it as defined in the standard. if writabe = 1 (true) that means the parameter is writable. And if writable is 0 (false) that means the parameter is non writable as mentioned in the standard (Annex A).

And it's a BLOBMSG_TYPE_STRING, because I will set it as string in the xml message. and even if there is a check I can make check on the string and I do not to convert to int (loose time). the Same thing for similar variables
+       [GET_PARAM_NAMES_FAULT] = { .name = "fault_code", .type = 
BLOBMSG_TYPE_STRING },
+};
+
+static int
+freecwmpd_handle_get_param_names(struct ubus_context *ctx, struct ubus_object 
*obj,
+                       struct ubus_request_data *req, const char *method,
+                       struct blob_attr *msg)
+{
+       struct blob_attr *tb[__GET_PARAM_NAMES_MAX];
+
+       blobmsg_parse(get_param_names_policy, 
ARRAY_SIZE(get_param_names_policy), tb,
+                     blob_data(msg), blob_len(msg));
+
+       if (!tb[GET_PARAM_NAMES_PARAM])
+               return UBUS_STATUS_INVALID_ARGUMENT;
+
+       freecwmp_log_message(NAME, L_NOTICE,
+                            "triggered ubus get_parameter_names for the parameter 
%s\n",
+                            blobmsg_data(tb[GET_PARAM_NAMES_PARAM]));
+
+       external_add_list_paramameter(blobmsg_data(tb[GET_PARAM_NAMES_PARAM]),
+                       tb[GET_PARAM_NAMES_WRITABLE]? 
blobmsg_data(tb[GET_PARAM_NAMES_WRITABLE]) : NULL,
Missing spaces.

+                       NULL,
+                       tb[GET_PARAM_NAMES_FAULT]? 
blobmsg_data(tb[GET_PARAM_NAMES_FAULT]) : NULL);
Missing spaces.

+
+       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("get_parameter_values", freecwmpd_handle_get_param_values, 
get_param_values_policy),
+       UBUS_METHOD("get_parameter_names", freecwmpd_handle_get_param_names, 
get_param_names_policy),
  };
static struct ubus_object_type main_object_type =
diff --git a/src/xml.c b/src/xml.c
index 612c0bb..c735244 100644
--- a/src/xml.c
+++ b/src/xml.c
@@ -51,6 +51,7 @@ static struct cwmp_namespaces
  const struct rpc_method rpc_methods[] = {
        { "SetParameterValues", xml_handle_set_parameter_values },
        { "GetParameterValues", xml_handle_get_parameter_values },
+       { "GetParameterNames", xml_handle_get_parameter_names },
        { "SetParameterAttributes", xml_handle_set_parameter_attributes },
        { "Download", xml_handle_download },
        { "FactoryReset", xml_handle_factory_reset },
@@ -716,7 +717,6 @@ int xml_handle_get_parameter_values(mxml_node_t *body_in,
                free(external_parameter->fault_code);
                free(external_parameter);
        }
-
  #ifdef ACS_MULTI
        b = mxmlFindElement(tree_out, tree_out, "ParameterList",
                            NULL, NULL, MXML_DESCEND);
@@ -736,6 +736,115 @@ out:
        return -1;
  }
+int xml_handle_get_parameter_names(mxml_node_t *body_in,
+                                   mxml_node_t *tree_in,
+                                   mxml_node_t *tree_out)
+{
+       mxml_node_t *n, *parameter_list, *b = body_in;
+       struct external_parameter *external_parameter;
+       char *parameter_name = NULL;
+       char *NextLevel = NULL;
All variables must be loverspace separated with underscore. Rename this
one to next_level.

+       char *c;
This variable is used only when ACS_MULTI is set. It should be defined
bellow just before it's used for the first time.

+       int counter = 0;
+
+       n = mxmlFindElement(tree_out, tree_out, "soap_env:Body",
+                           NULL, NULL, MXML_DESCEND);
+       if (!n) return -1;
+
+       n = mxmlNewElement(n, "cwmp:GetParameterNamesResponse");
+       if (!n) return -1;
+
+       n = mxmlNewElement(n, "ParameterList");
+       if (!n) return -1;
+
+#ifdef ACS_MULTI
+       mxmlElementSetAttr(n, "xsi:type", "soap_enc:Array");
+#endif
+
+       while (b) {
+               if (b && b->type == MXML_TEXT &&
+                       b->value.text.string &&
+                       b->parent->type == MXML_ELEMENT &&
+                       !strcmp(b->parent->value.element.name, 
"ParameterPath")) {
+                       parameter_name = b->value.text.string;
+               }
+               if (b && b->type == MXML_ELEMENT && /* added in order to 
support GetParameterNames with empty ParameterPath*/
Comments can't be here.

+                       !strcmp(b->value.element.name, "ParameterPath") &&
+                       !b->child) {
+                       parameter_name = "";
+               }
+               if (b && b->type == MXML_TEXT &&
+                       b->value.text.string &&
+                       b->parent->type == MXML_ELEMENT &&
+                       !strcmp(b->parent->value.element.name, "NextLevel")) {
+                       NextLevel = b->value.text.string;
+               }
+               b = mxmlWalkNext(b, body_in, MXML_DESCEND);
+       }
Add newline here.

+       if (parameter_name && NextLevel) {
+               if (!external_get_action("name", parameter_name, NextLevel)) {
+                       // got the parameter value via external script
+               } else {
+                       // error occurred when getting parameter value
+                       goto out;
+               }
This section needs some rework.

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

+
Remove newline.

+               external_parameter = list_entry(external_list_parameter.next, 
struct external_parameter, list);
+
+               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
Add braces and update comment.

+
+               n = mxmlNewElement(parameter_list, "ParameterInfoStruct");
+               if (!n) goto out;
+
+               n = mxmlNewElement(n, "Name");
+               if (!n) goto out;
+
+               n = mxmlNewText(n, 0, external_parameter->name);
+               if (!n) goto out;
+
+               n = n->parent->parent;
+               n = mxmlNewElement(n, "Writable");
+               if (!n) goto out;
+
+               n = mxmlNewText(n, 0, external_parameter->data);
+               if (!n) goto out;
+
+               counter++;
+
+               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);
This I have seen in several places already. This should go into a function then.

+       }
+
+#ifdef ACS_MULTI
+       b = mxmlFindElement(tree_out, tree_out, "ParameterList",
+                           NULL, NULL, MXML_DESCEND);
+       if (!b) goto out;
+
+       if (asprintf(&c, "cwmp:ParameterInfoStruct[%d]", counter) == -1)
+               goto out;
+
+       mxmlElementSetAttr(b, "soap_enc:arrayType", c);
+       FREE(c);
+#endif
+
+       return 0;
+
+out:
+       external_free_list_parameter();
+       return -1;
+}
+
  static int xml_handle_set_parameter_attributes(mxml_node_t *body_in,
                                               mxml_node_t *tree_in,
                                               mxml_node_t *tree_out) {
diff --git a/src/xml.h b/src/xml.h
index 8f63192..a9191eb 100644
--- a/src/xml.h
+++ b/src/xml.h
@@ -26,6 +26,10 @@ static int xml_handle_get_parameter_values(mxml_node_t 
*body_in,
                                           mxml_node_t *tree_in,
                                           mxml_node_t *tree_out);
+static int xml_handle_get_parameter_names(mxml_node_t *body_in,
+                                          mxml_node_t *tree_in,
+                                          mxml_node_t *tree_out);
+
  static int xml_handle_set_parameter_attributes(mxml_node_t *body_in,
                                               mxml_node_t *tree_in,
                                               mxml_node_t *tree_out);
--
1.7.4.1
Luka



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