Re: [PATCH 5/6] stkutil: Complete the TLV parsing/builder to support BIP commands
Hi Denis,
On 03/24/2011 03:32 PM, Denis Kenzior wrote:
Hi Philippe,
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.
All the above comments are just relevant. Thank you for having spent
time to point all these remarks and especially all the coding style
noncompliances. I will apply your comments in a new patch and separate
cleaning/typo modifications in another dedicated patch.
Here, the apn string is indeed extracted from the encoded buffer.
As the encoded apn just can't exceeed 100 bytes as per the spec and as
the extracted string is lesser than 100 bytes since we are removing the
octet length field , I considered that the string buffer was better
sized with 100 bytes than 127. But perhaps, I'm over thinking here...
Then just remove this structure and use a static buffer for the apn/nan...
};
@@ -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;
e.g. char name[100] here.
+/* 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?
In practice, the size (rx_remaining or tx_avail) can reach 65535 as the
buffer size it self given by the UICC is encoded by 2 bytes. I could
consider indeed to use at least an unsigned short, but to be homogeneous
with the type of the buffer size given by data.len, I preferred to go
with an unsigned int.
Stick with actual datatypes used on the wire. If this is an 8 bit
sequence with FF used to mean 255 bytes or more, then use that. The
logic that fills this structure will have to take care of that.
Besides, you use unsigned char in stk_response_receive_data and
stk_response_send_data, so might as well be consistent.
Use of unsigned int should be limited to the length field of the TLV,
and even then we might have to change this later for fields that are short.
Regards,
-Denis
Indeed the size to be returned in the Terminal response is encoded with
one byte (reason why you can find an unsigned char in the structures
stk_response_receive_data and stk_response_send_data).
I understand also the logic that fills the Data Length structure:
stk_tlv_builder_append_byte(tlv, MIN(*length, 255))
but I really need to store the real number of bytes remaining/available,
and this number can exceed 255. So, I would like to keep this data type
or at least go for an unsigned short.
Regarding the modifications I did in the enum section/structures, you wrote:
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.
Could you clarify with an example ?
Thanks.
Regards,
Philippe.
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH 5/6] stkutil: Complete the TLV parsing/builder to support BIP commands
Hi Philippe,
snip
Indeed the size to be returned in the Terminal response is encoded with
one byte (reason why you can find an unsigned char in the structures
stk_response_receive_data and stk_response_send_data).
I understand also the logic that fills the Data Length structure:
stk_tlv_builder_append_byte(tlv, MIN(*length, 255))
but I really need to store the real number of bytes remaining/available,
and this number can exceed 255. So, I would like to keep this data type
or at least go for an unsigned short.
Then go with unsigned short for now and I'll try to pay attention as to
why you think you need this on my next review.
Regarding the modifications I did in the enum section/structures, you
wrote:
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.
Could you clarify with an example ?
Just have a look at stkutil.h, there are plenty of examples.
e.g.
/*
* TS 101.220, Section 7.2, Card Application Toolkit assigned templates,
* These are the same as 3GPP 11.14 Sections 13.1 and 13.2
*/
enum stk_envelope_type {
or
/* TS 102.223 Section 9.4 */
enum stk_command_type {
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH 5/6] stkutil: Complete the TLV parsing/builder to support BIP commands
Hi Philippe,
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.
All the above comments are just relevant. Thank you for having spent
time to point all these remarks and especially all the coding style
noncompliances. I will apply your comments in a new patch and separate
cleaning/typo modifications in another dedicated patch.
Here, the apn string is indeed extracted from the encoded buffer.
As the encoded apn just can't exceeed 100 bytes as per the spec and as
the extracted string is lesser than 100 bytes since we are removing the
octet length field , I considered that the string buffer was better
sized with 100 bytes than 127. But perhaps, I'm over thinking here...
Then just remove this structure and use a static buffer for the apn/nan...
};
@@ -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;
e.g. char name[100] here.
+/* 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?
In practice, the size (rx_remaining or tx_avail) can reach 65535 as the
buffer size it self given by the UICC is encoded by 2 bytes. I could
consider indeed to use at least an unsigned short, but to be homogeneous
with the type of the buffer size given by data.len, I preferred to go
with an unsigned int.
Stick with actual datatypes used on the wire. If this is an 8 bit
sequence with FF used to mean 255 bytes or more, then use that. The
logic that fills this structure will have to take care of that.
Besides, you use unsigned char in stk_response_receive_data and
stk_response_send_data, so might as well be consistent.
Use of unsigned int should be limited to the length field of the TLV,
and even then we might have to change this later for fields that are short.
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH 5/6] stkutil: Complete the TLV parsing/builder to support BIP commands
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
[PATCH 5/6] stkutil: Complete the TLV parsing/builder to support BIP commands
---
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];
+ 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);
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;
- if (len == 0)
+ if ((len == 0) || (len 100))
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)
+ strcat((char *)apn-name, .);
+ }
+
+ apn-len = strlen((const char *)apn-name);
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;
+
+ 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,
+
