Re: [PATCH 11/19] stkutil: Add setup menu proactive command parser

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3998 bytes --]

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