Re: [PATCH 5/6] stkutil: Complete the TLV parsing/builder to support BIP commands

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Philippe,

On 03/22/2011 07:51 AM, Philippe Nunes wrote:
> ---
>  src/stkutil.c |  300 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  src/stkutil.h |  141 +++++++++++++++++++++++----
>  2 files changed, 402 insertions(+), 39 deletions(-)
> 
> diff --git a/src/stkutil.c b/src/stkutil.c
> index c64cb7a..b03dfb4 100644
> --- a/src/stkutil.c
> +++ b/src/stkutil.c
> @@ -1264,8 +1264,22 @@ static gboolean parse_dataobj_bearer_description(
>  
>  	data = comprehension_tlv_iter_get_data(iter);
>  	bd->type = data[0];
> -	bd->len = len - 1;
> -	memcpy(bd->pars, data + 1, bd->len);
> +
> +	if (bd->type == STK_BEARER_TYPE_GPRS_UTRAN) {
> +		if (len < 7)
> +			return FALSE;
> +
> +		bd->parameters.gprs.precedence = data[1];
> +		bd->parameters.gprs.delay = data[2];
> +		bd->parameters.gprs.reliability = data[3];
> +		bd->parameters.gprs.peak = data[4];
> +		bd->parameters.gprs.mean = data[5];
> +		bd->parameters.gprs.pdp_type = data[6];

Please do me a favor and insert an empty line here.  I like the blocks
to be grouped logically, especially the bigger ones.

> +		return TRUE;
> +	}
> +
> +	bd->parameters.other.len = len - 1;
> +	memcpy(bd->parameters.other.pars, data + 1, bd->parameters.other.len);
>  
>  	return TRUE;
>  }
> @@ -1356,7 +1370,11 @@ static gboolean parse_dataobj_other_address(
>  
>  	data = comprehension_tlv_iter_get_data(iter);
>  	oa->type = data[0];
> -	memcpy(&oa->addr, data + 1, len - 1);
> +
> +	if (oa->type == STK_ADDRESS_IPV4)
> +		memcpy(&oa->addr.ipv4, data + 1, 4);
> +	else
> +		memcpy(&oa->addr.ipv6, data + 1, 16);

You might want to be more paranoid here, don't assume that just because
oa->type != STK_ADDRESS_IPV4 it will be IPv6.

>  
>  	return TRUE;
>  }
> @@ -1604,16 +1622,33 @@ static gboolean parse_dataobj_esn(struct comprehension_tlv_iter *iter,
>  static gboolean parse_dataobj_network_access_name(
>  		struct comprehension_tlv_iter *iter, void *user)
>  {
> -	struct stk_network_access_name *nan = user;
> +	struct stk_network_access_name *apn = user;
>  	const unsigned char *data;
>  	unsigned int len = comprehension_tlv_iter_get_length(iter);
> +	size_t label_size;

Why is unsigned char not sufficient?

>  
> -	if (len == 0)
> +	if ((len == 0) || (len > 100))

Please don't do this, unnecessary parentheses are a distraction.  Learn
the precedence of operators or do like I do it: use a cheat sheet.

>  		return FALSE;
>  
>  	data = comprehension_tlv_iter_get_data(iter);
> -	nan->len = len;
> -	memcpy(nan->name, data, len);
> +	/*
> +	 * As specified in TS 23 003 Section 9
> +	 * The APN consists of one or more labels. Each label is coded as
> +	 * a one octet length field followed by that number of octets coded
> +	 * as 8 bit ASCII characters
> +	 */
> +
> +	while (len) {
> +		label_size = *data;
> +		data++;
> +		strncat((char *)apn->name, (char *)data, label_size);
> +		data += label_size;
> +		len -= label_size + 1;
> +		if (len)

doc/coding-style.txt, item M1

> +			strcat((char *)apn->name, ".");
> +	}
> +
> +	apn->len = strlen((const char *)apn->name);

Overall this code is pretty inefficient and not paranoid enough.

For paranoidness: you need to be making sure that label_size is sane and
not going to take you past your tlv length.

For efficiency: use memcpy and direct assignment instead of strncat.
While strncat is certainly fast, it still forces you to scan the entire
dest string to find its length.

>  
>  	return TRUE;
>  }
> @@ -3274,7 +3309,68 @@ static enum stk_command_parse_result parse_launch_browser(
>  				STK_DATA_OBJECT_TYPE_INVALID);
>  }
>  
> -/* TODO: parse_open_channel */
> +static void destroy_open_channel(struct stk_command *command)
> +{
> +	g_free(command->open_channel.alpha_id);
> +	g_free(command->open_channel.text_usr);
> +	g_free(command->open_channel.text_passwd);
> +}
> +
> +static enum stk_command_parse_result parse_open_channel(
> +					struct stk_command *command,
> +					struct comprehension_tlv_iter *iter)
> +{
> +	struct stk_command_open_channel *obj = &command->open_channel;
> +	enum stk_command_parse_result status;
> +
> +	if (command->qualifier >= 0x08)
> +		return STK_PARSE_RESULT_DATA_NOT_UNDERSTOOD;

I know you're only parsing a particular type of Open Channel, but it
might be a good idea to check this once that type has been determined.

> +
> +	if (command->src != STK_DEVICE_IDENTITY_TYPE_UICC)
> +		return STK_PARSE_RESULT_DATA_NOT_UNDERSTOOD;
> +
> +	if (command->dst != STK_DEVICE_IDENTITY_TYPE_TERMINAL)
> +		return STK_PARSE_RESULT_DATA_NOT_UNDERSTOOD;
> +
> +	command->destructor = destroy_open_channel;
> +
> +	/*
> +	 * parse the Open Channel data objects related to packet data service
> +	 * bearer
> +	 */
> +	status = parse_dataobj(iter,
> +				STK_DATA_OBJECT_TYPE_ALPHA_ID, 0,
> +				&obj->alpha_id,
> +				STK_DATA_OBJECT_TYPE_ICON_ID, 0,
> +				&obj->icon_id,
> +				STK_DATA_OBJECT_TYPE_BEARER_DESCRIPTION,
> +				DATAOBJ_FLAG_MANDATORY | DATAOBJ_FLAG_MINIMUM,
> +				&obj->bearer_desc,
> +				STK_DATA_OBJECT_TYPE_BUFFER_SIZE,
> +				DATAOBJ_FLAG_MANDATORY | DATAOBJ_FLAG_MINIMUM,
> +				&obj->buf_size,
> +				STK_DATA_OBJECT_TYPE_NETWORK_ACCESS_NAME, 0,
> +				&obj->network_name,
> +				STK_DATA_OBJECT_TYPE_OTHER_ADDRESS, 0,
> +				&obj->local_addr,
> +				STK_DATA_OBJECT_TYPE_TEXT, 0,
> +				&obj->text_usr,
> +				STK_DATA_OBJECT_TYPE_TEXT, 0,
> +				&obj->text_passwd,
> +				STK_DATA_OBJECT_TYPE_UICC_TE_INTERFACE, 0,
> +				&obj->uti,
> +				STK_DATA_OBJECT_TYPE_OTHER_ADDRESS, 0,
> +				&obj->data_dest_addr,
> +				STK_DATA_OBJECT_TYPE_TEXT_ATTRIBUTE, 0,
> +				&obj->text_attr,
> +				STK_DATA_OBJECT_TYPE_FRAME_ID, 0,
> +				&obj->frame_id,
> +				STK_DATA_OBJECT_TYPE_INVALID);
> +
> +	CHECK_TEXT_AND_ICON(obj->alpha_id, obj->icon_id.id);
> +
> +	return status;
> +}
>  
>  static void destroy_close_channel(struct stk_command *command)
>  {
> @@ -3291,7 +3387,8 @@ static enum stk_command_parse_result parse_close_channel(
>  	if (command->src != STK_DEVICE_IDENTITY_TYPE_UICC)
>  		return STK_PARSE_RESULT_DATA_NOT_UNDERSTOOD;
>  
> -	if (command->dst != STK_DEVICE_IDENTITY_TYPE_TERMINAL)
> +	if ((command->dst < STK_DEVICE_IDENTITY_TYPE_CHANNEL_1) ||
> +		(command->dst > STK_DEVICE_IDENTITY_TYPE_CHANNEL_7))

doc/coding-style.txt item M4

>  		return STK_PARSE_RESULT_DATA_NOT_UNDERSTOOD;
>  
>  	command->destructor = destroy_close_channel;
> @@ -3353,7 +3450,7 @@ static enum stk_command_parse_result parse_receive_data(
>  static void destroy_send_data(struct stk_command *command)
>  {
>  	g_free(command->send_data.alpha_id);
> -	g_free(command->send_data.data.array);
> +	g_free(command->send_data.tx_data.array);
>  }
>  
>  static enum stk_command_parse_result parse_send_data(
> @@ -3363,6 +3460,9 @@ static enum stk_command_parse_result parse_send_data(
>  	struct stk_command_send_data *obj = &command->send_data;
>  	enum stk_command_parse_result status;
>  
> +	if (command->qualifier > STK_SEND_DATA_IMMEDIATELY)
> +			return STK_PARSE_RESULT_DATA_NOT_UNDERSTOOD;
> +
>  	if (command->src != STK_DEVICE_IDENTITY_TYPE_UICC)
>  		return STK_PARSE_RESULT_DATA_NOT_UNDERSTOOD;
>  
> @@ -3378,7 +3478,7 @@ static enum stk_command_parse_result parse_send_data(
>  				&obj->icon_id,
>  				STK_DATA_OBJECT_TYPE_CHANNEL_DATA,
>  				DATAOBJ_FLAG_MANDATORY | DATAOBJ_FLAG_MINIMUM,
> -				&obj->data,
> +				&obj->tx_data,

tx_data is a bit redundant, since we're already in the send_data command.

>  				STK_DATA_OBJECT_TYPE_TEXT_ATTRIBUTE, 0,
>  				&obj->text_attr,
>  				STK_DATA_OBJECT_TYPE_FRAME_ID, 0,
> @@ -3737,6 +3837,8 @@ static enum stk_command_parse_result parse_command_body(
>  		return parse_language_notification(command, iter);
>  	case STK_COMMAND_TYPE_LAUNCH_BROWSER:
>  		return parse_launch_browser(command, iter);
> +	case STK_COMMAND_TYPE_OPEN_CHANNEL:
> +		return parse_open_channel(command, iter);
>  	case STK_COMMAND_TYPE_CLOSE_CHANNEL:
>  		return parse_close_channel(command, iter);
>  	case STK_COMMAND_TYPE_RECEIVE_DATA:
> @@ -4755,9 +4857,41 @@ static gboolean build_dataobj_bearer_description(struct stk_tlv_builder *tlv,
>  	if (bd->type == 0x00)
>  		return TRUE;
>  
> +	if (bd->type == STK_BEARER_TYPE_GPRS_UTRAN) {
> +

Please remove this empty line

> +		return stk_tlv_builder_open_container(tlv, cr, tag, FALSE) &&
> +		stk_tlv_builder_append_byte(tlv, bd->type) &&
> +		stk_tlv_builder_append_byte(tlv,
> +				bd->parameters.gprs.precedence) &&
> +		stk_tlv_builder_append_byte(tlv,
> +				bd->parameters.gprs.delay) &&
> +		stk_tlv_builder_append_byte(tlv,
> +				bd->parameters.gprs.reliability) &&
> +		stk_tlv_builder_append_byte(tlv,
> +				bd->parameters.gprs.peak) &&
> +		stk_tlv_builder_append_byte(tlv,
> +				bd->parameters.gprs.mean) &&
> +		stk_tlv_builder_append_byte(tlv,
> +				bd->parameters.gprs.pdp_type) &&
> +		stk_tlv_builder_close_container(tlv);

Please do me a favor and indent everything after the return at least one
more level.  Use a separate function if you have to.

> +	}
> +
>  	return stk_tlv_builder_open_container(tlv, cr, tag, FALSE) &&
>  		stk_tlv_builder_append_byte(tlv, bd->type) &&
> -		stk_tlv_builder_append_bytes(tlv, bd->pars, bd->len) &&
> +		stk_tlv_builder_append_bytes(tlv, bd->parameters.other.pars,
> +				bd->parameters.other.len) &&
> +		stk_tlv_builder_close_container(tlv);
> +}
> +
> +/* Described in TS 102.223 Section 8.53 */
> +static gboolean build_dataobj_channel_data(struct stk_tlv_builder *tlv,
> +						const void *data, gboolean cr)
> +{
> +	const struct stk_common_byte_array *cd = data;
> +	unsigned char tag = STK_DATA_OBJECT_TYPE_CHANNEL_DATA;
> +
> +	return stk_tlv_builder_open_container(tlv, cr, tag, TRUE) &&
> +		stk_tlv_builder_append_bytes(tlv, cd->array, cd->len) &&
>  		stk_tlv_builder_close_container(tlv);
>  }
>  
> @@ -4774,15 +4908,56 @@ static gboolean build_dataobj_channel_data_length(
>  		stk_tlv_builder_close_container(tlv);
>  }
>  
> +/* Described in TS 102.223 Section 8.55 */
> +static gboolean build_dataobj_buffer_size(struct stk_tlv_builder *tlv,
> +					const void *data, gboolean cr)
> +{
> +	const guint16 *buf_size = data;
> +	unsigned char tag = STK_DATA_OBJECT_TYPE_BUFFER_SIZE;
> +
> +	return stk_tlv_builder_open_container(tlv, cr, tag, FALSE) &&
> +		stk_tlv_builder_append_short(tlv, *buf_size) &&
> +		stk_tlv_builder_close_container(tlv);
> +}
> +
>  /* Described in TS 102.223 Section 8.56 */
>  static gboolean build_dataobj_channel_status(struct stk_tlv_builder *tlv,
>  						const void *data, gboolean cr)
>  {
> +	const struct stk_channel *channel = data;
>  	unsigned char tag = STK_DATA_OBJECT_TYPE_CHANNEL_STATUS;
> +	gboolean ok = FALSE;
>  
> -	return stk_tlv_builder_open_container(tlv, cr, tag, FALSE) &&
> -		stk_tlv_builder_append_bytes(tlv, data, 2) &&
> -		stk_tlv_builder_close_container(tlv);
> +	if (stk_tlv_builder_open_container(tlv, cr, tag, FALSE) == FALSE)
> +			return FALSE;
> +
> +	switch (channel->status) {
> +	case STK_CHANNEL_PACKET_DATA_SERVICE_NOT_ACTIVATED:
> +	case STK_CHANNEL_TCP_IN_CLOSED_STATE:
> +		ok = stk_tlv_builder_append_byte(tlv, channel->id) &&
> +			stk_tlv_builder_append_byte(tlv, 0x00);
> +		break;
> +	case STK_CHANNEL_PACKET_DATA_SERVICE_ACTIVATED:
> +	case STK_CHANNEL_TCP_IN_ESTABLISHED_STATE:
> +		ok = stk_tlv_builder_append_byte(tlv,
> +				channel->id | 0x80) &&
> +			stk_tlv_builder_append_byte(tlv, 0x00);
> +		break;
> +	case STK_CHANNEL_TCP_IN_LISTEN_STATE:
> +		ok = stk_tlv_builder_append_byte(tlv,
> +			channel->id | 0x40) &&
> +			stk_tlv_builder_append_byte(tlv, 0x00);
> +		break;
> +	case STK_CHANNEL_LINK_DROPPED:
> +		ok = stk_tlv_builder_append_byte(tlv, channel->id) &&
> +			stk_tlv_builder_append_byte(tlv, 0x05);
> +		break;
> +	}

I'd rather you use a two byte array and fill the contents in the switch
statement.  Then do the append_bytes after.

e.g.

switch(status)
case foo:
	data[0] = ...
	data[1] = ...
	break;
...
}

if (!stk_tlv_builder_append_byte(data[0]) ||
!stk_tlv_builder_append_byte(data[1])
	return FALSE;


> +
> +	if (!ok)
> +		return FALSE;
> +
> +	return stk_tlv_builder_close_container(tlv);
>  }
>  
>  /* Described in TS 102.223 Section 8.58 */
> @@ -4806,7 +4981,7 @@ static gboolean build_dataobj_other_address(struct stk_tlv_builder *tlv,
>  	case STK_ADDRESS_IPV4:
>  		ok = stk_tlv_builder_append_byte(tlv, addr->type) &&
>  			stk_tlv_builder_append_bytes(tlv,
> -					(const guint8 *) &addr->addr.ipv4, 4);
> +					(const guint8*) &addr->addr.ipv4, 4);

Why? The original version is just fine

>  		break;
>  	case STK_ADDRESS_IPV6:
>  		ok = stk_tlv_builder_append_byte(tlv, addr->type) &&
> @@ -5159,7 +5334,7 @@ static gboolean build_dataobj_registry_application_data(
>  	len = gsmlen;
>  	dcs = 0x04;
>  	if (name == NULL) {
> -		name = (guint8 *) g_convert((const gchar *) rad->name, -1,
> +		name = (guint8 *) g_convert((const gchar*) rad->name, -1,

And here

>  						"UCS-2BE", "UTF-8//TRANSLIT",
>  						NULL, &len, NULL);
>  		dcs = 0x08;
> @@ -5454,6 +5629,55 @@ static gboolean build_local_info(struct stk_tlv_builder *builder,
>  	return FALSE;
>  }
>  
> +static gboolean build_open_channel(struct stk_tlv_builder *builder,
> +					const struct stk_response *response)
> +{
> +	const struct stk_response_open_channel *open_channel =
> +		&response->open_channel;
> +
> +	/* insert channel identifier only in case of success */
> +	if (response->result.type == STK_RESULT_TYPE_SUCCESS) {
> +

Please remove this empty line, this is covered in doc/coding-style.txt
item M1.

> +		if (build_dataobj(builder, build_dataobj_channel_status,
> +						0, &open_channel->channel,
> +						NULL) != TRUE)
> +			return FALSE;
> +	}
> +
> +	return build_dataobj(builder,
> +				build_dataobj_bearer_description,
> +				0, &open_channel->bearer_desc,
> +				build_dataobj_buffer_size,
> +				0, &open_channel->buf_size,
> +				NULL);
> +}
> +
> +static gboolean build_receive_data(struct stk_tlv_builder *builder,
> +					const struct stk_response *response)
> +{
> +	const struct stk_response_receive_data *receive_data =
> +		&response->receive_data;
> +
> +	if (response->result.type == STK_RESULT_TYPE_SUCCESS) {
> +

And again an unneeded empty line ;)

> +		if (receive_data->rx_data.len) {
> +			if (build_dataobj(builder, build_dataobj_channel_data,
> +					DATAOBJ_FLAG_CR,
> +					&response->receive_data.rx_data,
> +					NULL) != TRUE)
> +				return FALSE;
> +		}
> +
> +		return build_dataobj(builder,
> +				build_dataobj_channel_data_length,
> +				DATAOBJ_FLAG_CR,
> +				&response->receive_data.data_left_length,
> +				NULL);
> +	}
> +
> +	return TRUE;
> +}
> +
>  const unsigned char *stk_pdu_from_response(const struct stk_response *response,
>  						unsigned int *out_length)
>  {
> @@ -5493,7 +5717,7 @@ const unsigned char *stk_pdu_from_response(const struct stk_response *response,
>  	 * Min = N.
>  	 *
>  	 * However comprehension required is set for many of the TLVs in
> -	 * TS 102 384 conformace tests so we set it per command and per
> +	 * TS 102 384 conformance tests so we set it per command and per

This really belongs in a separate patch

>  	 * data object type.
>  	 */
>  	tag = STK_DATA_OBJECT_TYPE_DEVICE_IDENTITIES;
> @@ -5582,6 +5806,7 @@ const unsigned char *stk_pdu_from_response(const struct stk_response *response,
>  	case STK_COMMAND_TYPE_SEND_DTMF:
>  	case STK_COMMAND_TYPE_LANGUAGE_NOTIFICATION:
>  	case STK_COMMAND_TYPE_LAUNCH_BROWSER:
> +	case STK_COMMAND_TYPE_CLOSE_CHANNEL:
>  		break;
>  	case STK_COMMAND_TYPE_SEND_USSD:
>  		ok = build_dataobj(&builder,
> @@ -5590,6 +5815,39 @@ const unsigned char *stk_pdu_from_response(const struct stk_response *response,
>  					&response->send_ussd.text,
>  					NULL);
>  		break;
> +	case STK_COMMAND_TYPE_OPEN_CHANNEL:
> +		/*
> +		 * return the channel identifier and the channel status
> +		 */
> +		ok = build_open_channel(&builder, response);
> +		break;
> +	case STK_COMMAND_TYPE_RECEIVE_DATA:
> +		/*
> +		 * return the requested data and the number of bytes
> +		 * remaining in the Rx buffer
> +		 */
> +		ok = build_receive_data(&builder, response);
> +		break;

I don't really see the point of these comments, especially here.
Comments should not describe something that is obvious and should try to
be local to where they are relevant.  If I peek inside
build_receive_data then this comment is really redundant.

> +	case STK_COMMAND_TYPE_SEND_DATA:
> +		/*
> +		 * return the number of bytes of empty space available
> +		 * in the Tx buffer
> +		 */
> +		if (response->result.type == STK_RESULT_TYPE_SUCCESS) {
> +			ok = build_dataobj(&builder,
> +					build_dataobj_channel_data_length,
> +					DATAOBJ_FLAG_CR,
> +					&response->send_data.empty_data_length,
> +					NULL);
> +		}
> +		break;

I suggest you use a dedicated function for builders that include objects
conditionally.

> +	case STK_COMMAND_TYPE_GET_CHANNEL_STATUS:
> +		ok = build_dataobj(&builder,
> +					build_dataobj_channel_status,
> +					DATAOBJ_FLAG_CR,
> +					&response->channel_status.channel,
> +					NULL);
> +		break;
>  	default:
>  		return NULL;
>  	};
> @@ -5738,7 +5996,7 @@ static gboolean build_envelope_event_download(struct stk_tlv_builder *builder,
>  		return build_dataobj(builder,
>  					build_dataobj_channel_status,
>  					DATAOBJ_FLAG_CR,
> -					&evt->data_available.channel_status,
> +					&evt->data_available.channel,
>  					build_dataobj_channel_data_length,
>  					DATAOBJ_FLAG_CR,
>  					&evt->data_available.channel_data_len,
> @@ -5747,7 +6005,7 @@ static gboolean build_envelope_event_download(struct stk_tlv_builder *builder,
>  		return build_dataobj(builder,
>  					build_dataobj_channel_status,
>  					DATAOBJ_FLAG_CR,
> -					&evt->channel_status.status,
> +					&evt->channel_status.channel,
>  					build_dataobj_bearer_description,
>  					DATAOBJ_FLAG_CR,
>  					&evt->channel_status.bearer_desc,
> @@ -6269,7 +6527,7 @@ char *stk_image_to_xpm(const unsigned char *img, unsigned int len,
>  
>  	/*
>  	 * space needed:
> -	 * 	header line
> +	 *	header line

Again, belongs in a separate cleanup patch

>  	 *	declaration and beginning of assignment line
>  	 *	values - max length of 19
>  	 *	colors - ncolors * (cpp + whitespace + deliminators + color)
> diff --git a/src/stkutil.h b/src/stkutil.h
> index f2df23f..241a807 100644
> --- a/src/stkutil.h
> +++ b/src/stkutil.h
> @@ -245,7 +245,7 @@ enum stk_result_type {
>  	STK_RESULT_TYPE_USER_REJECT =			0x22,
>  	STK_RESULT_TYPE_USER_CANCEL =			0x23,
>  	STK_RESULT_TYPE_TIMER_CONFLICT =		0x24,
> -	STK_RESULT_TYPE_CALL_CONTROL_TEMPORARY = 	0x25,
> +	STK_RESULT_TYPE_CALL_CONTROL_TEMPORARY =	0x25,

Separate cleanup patch please

>  	STK_RESULT_TYPE_BROWSER_TEMPORARY =		0x26,
>  	STK_RESULT_TYPE_MMS_TEMPORARY =			0x27,
>  
> @@ -426,13 +426,17 @@ enum stk_browser_termination_cause {
>  };
>  
>  enum stk_bearer_type {

Can you do me a favor and add the Section # for the enums / structs
you're modifying / introducing where needed.  We were supposed to be
doing this but somehow were not consistent enough over time.

> +	STK_BEARER_TYPE_CS =			0x01,
> +	STK_BEARER_TYPE_GPRS_UTRAN =		0x02,
>  	STK_BEARER_TYPE_DEFAULT =		0x03,
>  	STK_BEARER_TYPE_INDEPENDENT =		0x04,
>  	STK_BEARER_TYPE_BLUETOOTH =		0x05,
>  	STK_BEARER_TYPE_IRDA =			0x06,
>  	STK_BEARER_TYPE_RS232 =			0x07,
> -	STK_BEARER_TYPE_PACKET_DATA_SERVICE =	0x08,
> -	STK_BEARER_TYPE_I_WLAN =		0x0a,
> +	STK_BEARER_TYPE_TIA_EIA_IS_820 =	0x08,
> +	STK_BEARER_TYPE_UTRAN_WITH_EXT_PARAMS = 0x09,
> +	STK_BEARER_TYPE_I_WLAN =		0x0A,
> +	STK_BEARER_TYPE_EUTRAN_MAPPED_UTRAN =	0x0B,
>  	STK_BEARER_TYPE_USB =			0x10
>  };
>  
> @@ -587,6 +591,36 @@ enum stk_img_scheme {
>  	STK_IMG_SCHEME_TRANSPARENCY =	0x22,
>  };
>  
> +enum stk_transport_protocol_type {
> +	STK_TRANSPORT_PROTOCOL_UDP_CLIENT_REMOTE =	0x01,
> +	STK_TRANSPORT_PROTOCOL_TCP_CLIENT_REMOTE =	0x02,
> +	STK_TRANSPORT_PROTOCOL_TCP_SERVER =		0x03,
> +	STK_TRANSPORT_PROTOCOL_UDP_CLIENT_LOCAL =	0x04,
> +	STK_TRANSPORT_PROTOCOL_TCP_CLIENT_LOCAL =	0x05,
> +	STK_TRANSPORT_PROTOCOL_DIRECT =			0x06,
> +};
> +
> +enum stk_channel_status {
> +	STK_CHANNEL_PACKET_DATA_SERVICE_NOT_ACTIVATED = 0x00,
> +	STK_CHANNEL_PACKET_DATA_SERVICE_ACTIVATED =	0x01,
> +	STK_CHANNEL_TCP_IN_CLOSED_STATE =		0x02,
> +	STK_CHANNEL_TCP_IN_LISTEN_STATE =		0x03,
> +	STK_CHANNEL_TCP_IN_ESTABLISHED_STATE =		0x04,
> +	STK_CHANNEL_LINK_DROPPED =			0x05,
> +};
> +
> +enum stk_qualifier_open_channel {
> +	STK_OPEN_CHANNEL_ON_DEMAND =			0x00,
> +	STK_OPEN_CHANNEL_IMMEDIATE =			0x01,
> +	STK_OPEN_CHANNEL_AUTOMATIC_RECONNECTION =	0x02,
> +	STK_OPEN_CHANNEL_IMMEDIATE_IN_BACKGROUND_MODE =	0x04,
> +};
> +
> +enum stk_qualifier_send_data {
> +	STK_SEND_DATA_STORE_DATA =	0x00,
> +	STK_SEND_DATA_IMMEDIATELY =	0x01,
> +};
> +
>  /* For data object that only has a byte array with undetermined length */
>  struct stk_common_byte_array {
>  	unsigned char *array;
> @@ -849,18 +883,31 @@ struct stk_timing_advance {
>  	unsigned char advance;
>  };
>  
> -/*
> - * According to 102.223 Section 8.52 the length of CTLV is 1 byte. This means
> - * that the maximum size is 127 according to the rules of CTLVs. This size also
> - * includes bearer type for 1 byte, so the maxmimum size of bearer parameters
> - * is 126.
> - */
> -struct stk_bearer_description {
> -	unsigned char type;
> +/* Bearer parameters for GPRS/UTRAN Packet Service/E-UTRAN */
> +struct stk_gprs_bearer_parameters {
> +	unsigned char precedence;
> +	unsigned char delay;
> +	unsigned char reliability;
> +	unsigned char peak;
> +	unsigned char mean;
> +	unsigned char pdp_type;
> +};
> +
> +/* Generic bearer parameters */
> +struct stk_other_bearer_parameters {
>  	unsigned char pars[126];
>  	unsigned int len;
>  };
>  
> +/* Defined in TS 31.111 Section 8.52 */
> +struct stk_bearer_description {
> +	enum stk_bearer_type type;
> +	union {
> +		struct stk_gprs_bearer_parameters gprs;
> +		struct stk_other_bearer_parameters other;
> +	} parameters;

You can use an un-named union here and refer to the structures a bit
easier.  e.g. bs->gprs or bs->other.

> +};
> +
>  /*
>   * According to 102.223 Section 8.57 the length of CTLV is 1 byte. This means
>   * that the maximum size is 127 according to the rules of CTLVs.
> @@ -885,7 +932,7 @@ struct stk_other_address {
>  
>  /* Defined in TS 102.223 Section 8.59 */
>  struct stk_uicc_te_interface {
> -	unsigned char protocol;
> +	enum stk_transport_protocol_type protocol;
>  	unsigned short port;
>  };
>  
> @@ -950,11 +997,12 @@ struct stk_remote_entity_address {
>  };
>  
>  /*
> - * According to 102.223 Section 8.70 the length of CTLV is 1 byte. This means
> - * that the maximum size is 127 according to the rules of CTLVs.
> + * According to 102.223 Section 8.70 the length of CTLV is 1 byte.
> + * According to the 23.003 Section 9.0, the APN has, after encoding,
> + * a maximum length of 100 octets
>   */
>  struct stk_network_access_name {
> -	unsigned char name[127];
> +	unsigned char name[100];
>  	unsigned char len;

This part really makes no sense.  Since you parse it into a string,
might as well just use that.

>  };
>  
> @@ -1250,6 +1298,21 @@ struct stk_command_launch_browser {
>  	char *text_passwd;
>  };
>  
> +struct stk_command_open_channel {
> +	char *alpha_id;
> +	struct stk_icon_id icon_id;
> +	struct stk_bearer_description bearer_desc;
> +	unsigned short buf_size;
> +	struct stk_network_access_name network_name;

E.g. char *network_name;

> +	struct stk_other_address local_addr;
> +	char *text_usr;
> +	char *text_passwd;
> +	struct stk_uicc_te_interface uti;
> +	struct stk_other_address data_dest_addr;
> +	struct stk_text_attribute text_attr;
> +	struct stk_frame_id frame_id;
> +};
> +
>  struct stk_command_close_channel {
>  	char *alpha_id;
>  	struct stk_icon_id icon_id;
> @@ -1268,7 +1331,7 @@ struct stk_command_receive_data {
>  struct stk_command_send_data {
>  	char *alpha_id;
>  	struct stk_icon_id icon_id;
> -	struct stk_common_byte_array data;
> +	struct stk_common_byte_array tx_data;

As I mentioned previously, I feel this is a bit redundant.

>  	struct stk_text_attribute text_attr;
>  	struct stk_frame_id frame_id;
>  };
> @@ -1368,6 +1431,7 @@ struct stk_command {
>  		struct stk_command_send_dtmf send_dtmf;
>  		struct stk_command_language_notification language_notification;
>  		struct stk_command_launch_browser launch_browser;
> +		struct stk_command_open_channel open_channel;
>  		struct stk_command_close_channel close_channel;
>  		struct stk_command_receive_data receive_data;
>  		struct stk_command_send_data send_data;
> @@ -1406,6 +1470,24 @@ struct stk_ussd_text {
>  	int len;
>  };
>  
> +struct stk_channel {
> +	unsigned char id;
> +	enum stk_channel_status status;
> +};
> +
> +struct stk_channel_data {
> +	struct stk_common_byte_array data;
> +	union{

Forgot a space after union

> +		/* The number of bytes remaining in the Rx buffer */
> +		unsigned int data_left_length;

So maybe rx_remaining is a better name?

> +		/*
> +		 * The number of bytes of empty space available
> +		 * in the Tx buffer
> +		 */
> +		unsigned int empty_data_length;

And tx_avail here?

Then you don't need the comments ;)

Also, why do you use unsigned int instead of unsigned char?

> +	};
> +};
> +
>  struct stk_response_get_inkey {
>  	struct stk_answer_text text;
>  	struct stk_duration duration;
> @@ -1481,6 +1563,25 @@ struct stk_response_send_ussd {
>  	struct stk_ussd_text text;
>  };
>  
> +struct stk_response_open_channel {
> +	struct stk_channel channel;
> +	struct stk_bearer_description bearer_desc;
> +	unsigned short buf_size;
> +};
> +
> +struct stk_response_receive_data {
> +	struct stk_common_byte_array rx_data;
> +	unsigned char data_left_length;
> +};
> +
> +struct stk_response_send_data {
> +	unsigned char empty_data_length;
> +};
> +
> +struct stk_response_channel_status {
> +	struct stk_channel channel;
> +};
> +
>  struct stk_response {
>  	unsigned char number;
>  	unsigned char type;
> @@ -1511,6 +1612,10 @@ struct stk_response {
>  		struct stk_response_generic language_notification;
>  		struct stk_response_generic launch_browser;
>  		struct stk_response_send_ussd send_ussd;
> +		struct stk_response_open_channel open_channel;
> +		struct stk_response_receive_data receive_data;
> +		struct stk_response_send_data send_data;
> +		struct stk_response_channel_status channel_status;
>  	};
>  
>  	void (*destructor)(struct stk_response *response);
> @@ -1596,11 +1701,11 @@ struct stk_envelope_event_download {
>  			enum stk_browser_termination_cause cause;
>  		} browser_termination;
>  		struct {
> -			unsigned char channel_status[2];
> +			struct stk_channel channel;
>  			unsigned int channel_data_len;
>  		} data_available;
>  		struct {
> -			unsigned char status[2];
> +			struct stk_channel channel;
>  			struct stk_bearer_description bearer_desc;
>  			struct stk_other_address address;
>  		} channel_status;

Regards,
-Denis