Hi Yang,
> @@ -281,8 +281,9 @@ static gboolean parse_dataobj_alpha_id(struct
> comprehension_tlv_iter *iter, char *utf8;
>
> len = comprehension_tlv_iter_get_length(iter);
> - if (len < 1)
> - return FALSE;
> +
> + if (len == 0)
> + return TRUE;
>
> data = comprehension_tlv_iter_get_data(iter);
> utf8 = sim_string_to_utf8(data, len);
There is no mention that the Alpha identifier can be present and empty, so
returning TRUE on a len 0 is not actually correct. The only exception so far
seems to be text string which explicitly allows NULL strings.
> @@ -373,8 +374,10 @@ static gboolean parse_dataobj_item(struct
> comprehension_tlv_iter *iter, char *utf8;
>
> len = comprehension_tlv_iter_get_length(iter);
> - if (len < 2)
> - return FALSE;
> +
> + if (len == 0)
> + return TRUE;
> +
>
Same with this one...
> data = comprehension_tlv_iter_get_data(iter);
>
> @@ -2006,6 +2009,7 @@ static gboolean parse_dataobj(struct
> comprehension_tlv_iter *iter, GSList *l;
> va_list args;
> gboolean minimum_set = TRUE;
> + GSList **list = NULL;
>
> va_start(args, type);
>
> @@ -2022,29 +2026,74 @@ static gboolean parse_dataobj(struct
> comprehension_tlv_iter *iter, entries = g_slist_prepend(entries, entry);
> }
>
> -
> if (comprehension_tlv_iter_next(iter) != TRUE)
> goto out;
>
> entries = g_slist_reverse(entries);
>
> - for (l = entries; l; l = l->next) {
> + for (l = entries; l;) {
> dataobj_handler handler;
> struct dataobj_handler_entry *entry = l->data;
> + unsigned short tag;
> + gboolean ret;
> + void *dataobj;
>
> handler = handler_for_type(entry->type);
> - if (handler == NULL)
> + if (handler == NULL) {
> + l = l->next;
> continue;
> + }
>
> - if (comprehension_tlv_iter_get_tag(iter) == entry->type) {
> - if (handler(iter, entry->data))
> - entry->parsed = TRUE;
> - if (comprehension_tlv_iter_next(iter) == FALSE)
> - break;
> + tag = comprehension_tlv_iter_get_tag(iter);
> + if (tag != entry->type) {
> + l = l->next;
> + continue;
> }
> +
> + if (tag == STK_DATA_OBJECT_TYPE_PROVISIONING_FILE_REFERENCE ||
> + tag == STK_DATA_OBJECT_TYPE_ITEM) {
> +
> + if (tag == STK_DATA_OBJECT_TYPE_ITEM)
> + dataobj = g_try_new0(struct stk_item, 1);
> + else
> + dataobj = g_try_new0(struct stk_file, 1);
> +
> + if (!dataobj)
> + goto out;
> +
> + if (!list)
> + list = entry->data;
> +
> + ret = handler(iter, dataobj);
> + } else
> + ret = handler(iter, entry->data);
> +
> + if (ret)
> + entry->parsed = TRUE;
> +
> + if (tag == STK_DATA_OBJECT_TYPE_PROVISIONING_FILE_REFERENCE ||
> + tag == STK_DATA_OBJECT_TYPE_ITEM) {
> +
> + if (tag == STK_DATA_OBJECT_TYPE_ITEM) {
> + struct stk_item *item = dataobj;
> + /* either return is FALSE or item is empty */
> + if (item->id == 0)
> + g_free(item);
> + else
> + *list = g_slist_prepend(*list, item);
> + } else
> + *list = g_slist_prepend(*list, dataobj);
> + } else
> + l = l->next;
> +
> + if (comprehension_tlv_iter_next(iter) == FALSE)
> + break;
> }
>
I really don't like this. I understand why you're doing it this way, but lets
try to make this more elegant. Either add another FLAG saying that the items
are actually in a list, or parse the item list separately.
e.g.
parse_dataobj(iter, STK_DATA_OBJECT_TYPE_ALPHA_ID,
DATAOBJ_FLAG_MANDATORY | DATAOBJ_FLAG_MINIMUM,
&obj->alpha_id,
STK_DATA_OBJECT_TYPE_ITEM,
DATAOBJ_FLAG_MANDATORY | DATAOBJ_FLAG_MINIMUM |
DATAOBJ_FLAG_LIST,
&obj->items, ...
or
/* Parse only until item list */
parse_dataobj(iter, STK_DATA_OBJECT_TYPE_ALPHA_ID,
DATAOBJ_FLAG_MANDATORY | DATAOBJ_FLAG_MINIMUM,
&obj->alpha_id,
STK_DATA_OBJECT_TYPE_ITEM,
STK_DATA_OBJECT_TYPE_INVALID);
/* Manually parse item list */
...
/* Parse rest */
...
Regards,
-Denis
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
