Re: [PATCH 3/4] Add parser for file list objects

[Marcel Holtmann <marcel@holtmann.org>]

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

Hi Yang,

> ---
>  src/stkutil.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>  src/stkutil.h |    6 ++++++
>  2 files changed, 48 insertions(+), 0 deletions(-)
> 
> diff --git a/src/stkutil.c b/src/stkutil.c
> index ceba2d5..9f3bc0b 100644
> --- a/src/stkutil.c
> +++ b/src/stkutil.c
> @@ -406,6 +406,46 @@ static gboolean parse_dataobj_tone(struct comprehension_tlv_iter *iter,
>  	return TRUE;
>  }
>  
> +/* Defined in TS 102.223 Section 8.18 */
> +static gboolean parse_dataobj_file_list(struct comprehension_tlv_iter *iter,
> +						void *user)
> +{
> +	GSList **fl = user;
> +	const unsigned char *data;
> +	unsigned int len;
> +	unsigned int i;
> +	unsigned int start = 1;
> +	struct stk_file *sf;
> +
> +	if (comprehension_tlv_iter_get_tag(iter) !=
> +			STK_DATA_OBJECT_TYPE_FILE_LIST)
> +		return FALSE;
> +
> +	len = comprehension_tlv_iter_get_length(iter);
> +	if (len < 5)
> +		return FALSE;
> +
> +	data = comprehension_tlv_iter_get_data(iter);
> +
> +	if (data[start] != 0x3f)
> +		return FALSE;
> +
> +	for (i = start + 4; i <= len; i += 2) {
> +		if ((data[i] == 0x3f) || (i == len)) {
> +			sf = g_new0(struct stk_file, 1);
> +			sf->file = g_malloc(i-start);
> +			memcpy(sf->file, data+start, i-start);
> +			sf->len = i - start;
> +			*fl = g_slist_prepend(*fl, sf);
> +			start = i;
> +		}

doing a simple continue statement is way better.

	if (data[i] != 0x3f && i != len)
		continue;

Also there are not extra braces needed around expression in an if
statement. And i-start would have to be i - start.

When using g_new0 and g_malloc you result in exit on OOM. We try to
avoid that whenever possible. So I prefer using g_try_new0 and
g_try_malloc except there is a reason to just accept exit on OOM.

Personally the whole counting looks pretty complicated. I think it needs
either some documentation or made simpler.

Regards

Marcel