Return-Path: <linux-bluetooth-owner@vger.kernel.org>
MIME-Version: 1.0
In-Reply-To: <20171218203551.6142-1-inga.stotland@intel.com>
References: <20171218203551.6142-1-inga.stotland@intel.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Tue, 19 Dec 2017 09:46:34 -0200
Message-ID: <CABBYNZKA4XnssReMakV3dTGxMydX1O+j+qeLciVJqhyFenkGuw@mail.gmail.com>
Subject: Re: [PATCH BlueZ] mesh:meshctl: make model parsing more manageable
To: Inga Stotland <inga.stotland@intel.com>
Cc: "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
List-ID: <linux-bluetooth.vger.kernel.org>

Hi Inga,

On Mon, Dec 18, 2017 at 6:35 PM, Inga Stotland <inga.stotland@intel.com> wrote:
> Extract functionality for finding an existing model from
> parse_configuration_models() into new function find_configured_model().
> This removes confusing logic from overloaded implementation of
> parse_configuration_models().
> ---
>  mesh/prov-db.c | 54 ++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 42 insertions(+), 12 deletions(-)
>
> diff --git a/mesh/prov-db.c b/mesh/prov-db.c
> index 04803a5c8..8a7b47f6e 100644
> --- a/mesh/prov-db.c
> +++ b/mesh/prov-db.c
> @@ -371,14 +371,49 @@ static bool parse_bindings(struct mesh_node *node, int ele_idx,
>         return true;
>  }
>
> -static bool parse_configuration_models(struct mesh_node *node, int ele_idx,
> -               json_object *jmodels, uint32_t target_id, json_object **jtarget)
> +static json_object* find_configured_model(struct mesh_node *node, int ele_idx,
> +                                      json_object *jmodels, uint32_t target_id)
>  {
>         int model_cnt;
>         int i;
>
> -       if (jtarget)
> -               *jtarget = NULL;
> +       model_cnt = json_object_array_length(jmodels);
> +
> +       for (i = 0; i < model_cnt; ++i) {
> +               json_object *jmodel;
> +               json_object *jvalue;
> +               char *str;
> +               int len;
> +               uint32_t model_id;
> +
> +               jmodel = json_object_array_get_idx(jmodels, i);
> +
> +               json_object_object_get_ex(jmodel, "modelId", &jvalue);
> +               str = (char *)json_object_get_string(jvalue);
> +
> +               len = strlen(str);
> +
> +               if (len != 4 && len != 8)
> +                       return NULL;
> +
> +               if (sscanf(str, "%08x", &model_id) != 1)
> +                       return NULL;
> +
> +               if (len == 4)
> +                       model_id += 0xffff0000;
> +
> +               if (model_id == target_id)
> +                       return jmodel;
> +       }
> +
> +       return NULL;
> +}
> +
> +static bool parse_configuration_models(struct mesh_node *node, int ele_idx,
> +                                                       json_object *jmodels)
> +{
> +       int model_cnt;
> +       int i;
>
>         model_cnt = json_object_array_length(jmodels);
>
> @@ -405,11 +440,6 @@ static bool parse_configuration_models(struct mesh_node *node, int ele_idx,
>                 if (len == 4)
>                         model_id += 0xffff0000;
>
> -               if (jtarget && model_id == target_id) {
> -                       *jtarget = jmodel;
> -                       return true;
> -               }
> -
>                 json_object_object_get_ex(jmodel, "bind", &jarray);
>                 if (jarray && !parse_bindings(node, ele_idx, model_id, jarray))
>                         return false;
> @@ -468,7 +498,7 @@ static bool parse_configuration_elements(struct mesh_node *node,
>                 if (!jmodels)
>                         continue;
>
> -               if(!parse_configuration_models(node, index, jmodels, 0, NULL))
> +               if(!parse_configuration_models(node, index, jmodels))
>                         return false;
>         }
>         return true;
> @@ -1011,8 +1041,8 @@ static json_object *get_jmodel_obj(struct mesh_node *node, uint8_t ele_idx,
>                 jmodels = json_object_new_array();
>                 json_object_object_add(jelement, "models", jmodels);
>         } else {
> -               parse_configuration_models(node, ele_idx, jmodels,
> -                                                       model_id, &jmodel);
> +               jmodel = find_configured_model(node, ele_idx, jmodels,
> +                                                               model_id);
>         }
>
>         if (!jmodel) {
> --
> 2.13.6

While this looks fine I would suggest we start putting these pieces in
a shared library if we really intend these to be reused.

-- 
Luiz Augusto von Dentz