Skip to contentSkip to conversation

stkutil: Fix the assignment to minimum set

PATCH In review Yang Gu 4 participants 11 messages Apr 26, 2010 – Apr 27, 2010

lore
Share & tools
lore

Yang Gu

author
#0 lore raw

PATCH 1/6 stkutil: Fix the assignment to minimum set

---
 src/stkutil.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
src/stkutil.c +1 -1
index a457e36..b4279c1 100644
@@ -2053,7 +2053,7 @@ static gboolean parse_dataobj(struct comprehension_tlv_iter *iter,
 
 		if ((entry->flags & DATAOBJ_FLAG_MINIMUM) &&
 				entry->parsed == FALSE)
-			minimum_set = TRUE;
+			minimum_set = FALSE;
 	}
 
 	g_slist_foreach(entries, (GFunc)g_free, NULL);
Signature · 1 line
1.7.0.4
author
#1 lore raw

PATCH 2/6 stkutil: Move advance of ctlv to decicated proactive commands parser

Not all the proactive commands have comprehension tlv other than command details and device identities. So the appropriate code should be moved to decicated parser if needed.

---
 src/stkutil.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)
src/stkutil.c +12 -3
index b4279c1..9f9b1a6 100644
@@ -2079,6 +2079,9 @@ static gboolean parse_display_text(struct stk_command *command,
 	if (command->dst != STK_DEVICE_IDENTITY_TYPE_DISPLAY)
 		return FALSE;
 
+	if (comprehension_tlv_iter_next(iter) != TRUE)
+		return FALSE;
+
 	obj->frame_id = 0xFF;
 
 	ret = parse_dataobj(iter, STK_DATA_OBJECT_TYPE_TEXT,
@@ -2121,6 +2124,9 @@ static gboolean parse_get_inkey(struct stk_command *command,
 	if (command->dst != STK_DEVICE_IDENTITY_TYPE_TERMINAL)
 		return FALSE;
 
+	if (comprehension_tlv_iter_next(iter) != TRUE)
+		return FALSE;
+
 	obj->frame_id = 0xFF;
 
 	ret = parse_dataobj(iter, STK_DATA_OBJECT_TYPE_TEXT,
@@ -2165,6 +2171,9 @@ static gboolean parse_get_input(struct stk_command *command,
 	if (command->dst != STK_DEVICE_IDENTITY_TYPE_TERMINAL)
 		return FALSE;
 
+	if (comprehension_tlv_iter_next(iter) != TRUE)
+		return FALSE;
+
 	ret = parse_dataobj(iter, STK_DATA_OBJECT_TYPE_TEXT,
 				DATAOBJ_FLAG_MANDATORY | DATAOBJ_FLAG_MINIMUM,
 				&obj->text,
@@ -2210,6 +2219,9 @@ static gboolean parse_send_sms(struct stk_command *command,
 	if (command->dst != STK_DEVICE_IDENTITY_TYPE_NETWORK)
 		return FALSE;
 
+	if (comprehension_tlv_iter_next(iter) != TRUE)
+		return FALSE;
+
 	ret = parse_dataobj(iter, STK_DATA_OBJECT_TYPE_ALPHA_ID, 0,
 				&obj->alpha_id,
 				STK_DATA_OBJECT_TYPE_ADDRESS, 0,
@@ -2295,9 +2307,6 @@ struct stk_command *stk_command_new_from_pdu(const unsigned char *pdu,
 	command->src = data[0];
 	command->dst = data[1];
 
-	if (comprehension_tlv_iter_next(&iter) != TRUE)
-		return FALSE;
-
 	switch (command->type) {
 	case STK_COMMAND_TYPE_DISPLAY_TEXT:
 		ok = parse_display_text(command, &iter);
Signature · 1 line
1.7.0.4
#2 lore raw

Re: [PATCH 2/6] stkutil: Move advance of ctlv to decicated proactive commands parser

Hi Yang,

Quoted 3 lines · Yang Gu

Not all the proactive commands have comprehension tlv other than command details and device identities. So the appropriate code should be moved to decicated parser if needed.

Maybe parse_dataobj() would be a better place?

Regards

#3 lore raw

Re: [PATCH 2/6] stkutil: Move advance of ctlv to decicated proactive commands parser

Hi Andrew,

Quoted 8 lines

Hi Yang,

Quoted 3 lines · Yang Gu

Not all the proactive commands have comprehension tlv other than command details and device identities. So the appropriate code should be moved to decicated parser if needed.

Maybe parse_dataobj() would be a better place?

Agreed, lets not repeat this in every single command parser.

Regards,
-Denis

#4 lore raw

RE: [PATCH 2/6] stkutil: Move advance of ctlv to decicated proactive commands parser

Hi Andrew and Denis,

Quoted 12 lines

-----Original Message-----
From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)ofono.org] On Behalf Of Denis Kenzior
Sent: Monday, April 26, 2010 11:39 PM
To: ofono(a)ofono.org
Subject: Re: [PATCH 2/6] stkutil: Move advance of ctlv to decicated proactive commands parser

Hi Andrew,

Quoted 2 lines

Hi Yang,

Quoted 8 lines
Quoted 6 lines · Yang Gu
Quoted 3 lines

Not all the proactive commands have comprehension tlv other than command details and device identities. So the appropriate code should be moved to decicated parser if needed.

Maybe parse_dataobj() would be a better place?

Agreed, lets not repeat this in every single command parser.

Really good advice:) I will modify the patches.

Quoted 7 lines

Regards,
-Denis
_______________________________________________
ofono mailing list
ofono(a)ofono.org
http://lists.ofono.org/listinfo/ofono

Regards,
-Yang

author
#5 lore raw

PATCH 3/6 stkutil: Use struct to represent frame id

In reply to Yang Gu

Use a field has_id to indicate if we have frame id or not. Originally we use 0xFF to indicate we don't have frame id. But in that way, we have to initialize each test case of proactive command when it doesn't have this comprehension tlv to be 0xFF. To use has_id will save this effort, and keep consistency with other implementations.

---
 src/stkutil.c |   13 +++----------
 src/stkutil.h |   12 +++++++++---
 2 files changed, 12 insertions(+), 13 deletions(-)
src/stkutil.c +3 -10
index 9f9b1a6..2b43e85 100644
@@ -1635,7 +1635,7 @@ static gboolean parse_dataobj_frames_info(struct comprehension_tlv_iter *iter,
 static gboolean parse_dataobj_frame_id(struct comprehension_tlv_iter *iter,
 					void *user)
 {
-	unsigned char *frame_id = user;
+	struct stk_frame_id *fi = user;
 	const unsigned char *data;
 
 	if (comprehension_tlv_iter_get_length(iter) != 1)
@@ -1646,7 +1646,8 @@ static gboolean parse_dataobj_frame_id(struct comprehension_tlv_iter *iter,
 	if (data[0] >= 0x10)
 		return FALSE;
 
-	*frame_id = data[0];
+	fi->has_id = TRUE;
+	fi->id = data[0];
 
 	return TRUE;
 }
@@ -2082,8 +2083,6 @@ static gboolean parse_display_text(struct stk_command *command,
 	if (comprehension_tlv_iter_next(iter) != TRUE)
 		return FALSE;
 
-	obj->frame_id = 0xFF;
-
 	ret = parse_dataobj(iter, STK_DATA_OBJECT_TYPE_TEXT,
 				DATAOBJ_FLAG_MANDATORY | DATAOBJ_FLAG_MINIMUM,
 				&obj->text,
@@ -2127,8 +2126,6 @@ static gboolean parse_get_inkey(struct stk_command *command,
 	if (comprehension_tlv_iter_next(iter) != TRUE)
 		return FALSE;
 
-	obj->frame_id = 0xFF;
-
 	ret = parse_dataobj(iter, STK_DATA_OBJECT_TYPE_TEXT,
 				DATAOBJ_FLAG_MANDATORY | DATAOBJ_FLAG_MINIMUM,
 				&obj->text,
@@ -2163,8 +2160,6 @@ static gboolean parse_get_input(struct stk_command *command,
 	struct stk_command_get_input *obj = &command->get_input;
 	gboolean ret;
 
-	obj->frame_id = 0xFF;
-
 	if (command->src != STK_DEVICE_IDENTITY_TYPE_UICC)
 		return FALSE;
 
@@ -2211,8 +2206,6 @@ static gboolean parse_send_sms(struct stk_command *command,
 	struct gsm_sms_tpdu tpdu;
 	gboolean ret;
 
-	obj->frame_id = 0xFF;
-
 	if (command->src != STK_DEVICE_IDENTITY_TYPE_UICC)
 		return FALSE;
 
src/stkutil.h +9 -3
index 5d2a818..cbb6442 100644
@@ -728,6 +728,12 @@ struct stk_frames_info {
 	unsigned int len;
 };
 
+/* Defined in TS 102.223 Section 8.80 */
+struct stk_frame_id {
+	ofono_bool_t has_id;
+	unsigned char id;
+};
+
 /*
  * According to 102.223 Section 8.82 the length of CTLV is 1 byte. This means
  * that the maximum size is 127 according to the rules of CTLVs.
@@ -789,7 +795,7 @@ struct stk_command_display_text {
 	ofono_bool_t immediate_response;
 	struct stk_duration duration;
 	struct stk_text_attribute text_attribute;
-	unsigned char frame_id; /* Values 0x10 to 0xFF reserved */
+	struct stk_frame_id frame_id;
 };
 
 struct stk_command_get_input {
@@ -798,7 +804,7 @@ struct stk_command_get_input {
 	char *default_text;
 	struct stk_icon_id icon_id;
 	struct stk_text_attribute text_attribute;
-	unsigned char frame_id; /* Values 0x10 to 0xFF reserved */
+	struct stk_frame_id frame_id;
 };
 
 struct stk_command_send_sms {
@@ -807,7 +813,7 @@ struct stk_command_send_sms {
 	struct sms gsm_sms;
 	struct stk_icon_id icon_id;
 	struct stk_text_attribute text_attribute;
-	unsigned char frame_id; /* Values 0x10 to 0xFF reserved */
+	struct stk_frame_id frame_id;
 };
 
 struct stk_command {
Signature · 1 line
1.7.0.4
#6 lore raw

Re: [PATCH 3/6] stkutil: Use struct to represent frame id

Hi Yang,

Quoted 9 lines

Use a field has_id to indicate if we have frame id or not. Originally we
use 0xFF to indicate we don't have frame id. But in that way, we have to initialize each test case of proactive command when it doesn't have this comprehension tlv to be 0xFF. To use has_id will save this effort, and keep consistency with other implementations.

---
 src/stkutil.c |   13 +++----------
 src/stkutil.h |   12 +++++++++---
 2 files changed, 12 insertions(+), 13 deletions(-)

I'm fine with this one, can you rework #2 and resubmit along with this one?

Regards,
-Denis

author
#7 lore raw

PATCH 4/6 test-stkutil: Refactor the implementation to test display text

In reply to Yang Gu
---
 unit/test-stkutil.c |  160 ++++++++++++++++++++++++++++++++++----------------
 1 files changed, 109 insertions(+), 51 deletions(-)
unit/test-stkutil.c +109 -51
index 2bea19b..0961368 100644
@@ -35,15 +35,72 @@
 #include "smsutil.h"
 #include "stkutil.h"
 
+static gboolean g_mem_equal(const unsigned char *v1, const unsigned char *v2,
+				unsigned int len)
+{
+	unsigned int i;
+
+	for (i = 0; i < len; i++)
+		if (v1[i] != v2[i])
+			return FALSE;
+	return TRUE;
+}
+
+static inline void check_common_bool(const ofono_bool_t command,
+					const ofono_bool_t test)
+{
+	g_assert(command == test);
+}
+
+/* Defined in TS 102.223 Section 8.8 */
+static void check_duration(const struct stk_duration *command,
+					const struct stk_duration *test)
+{
+	g_assert(command->unit == test->unit);
+	g_assert(command->interval == test->interval);
+}
+
+/* Defined in TS 102.223 Section 8.15 */
+static inline void check_text(const char *command, const char *test)
+{
+	g_assert(g_str_equal(command, test));
+}
+
+/* Defined in TS 102.223 Section 8.31 */
+static void check_icon_id(const struct stk_icon_id *command,
+					const struct stk_icon_id *test)
+{
+	g_assert(command->id == test->id);
+	g_assert(command->qualifier == test->qualifier);
+}
+
+/* Defined in TS 102.223 Section 8.72 */
+static void check_text_attr(const struct stk_text_attribute *command,
+					const struct stk_text_attribute *test)
+{
+	g_assert(command->len == test->len);
+	g_assert(g_mem_equal(command->attributes, test->attributes, test->len));
+}
+
+/* Defined in TS 102.223 Section 8.80 */
+static void check_frame_id(const struct stk_frame_id *command,
+					const struct stk_frame_id *test)
+{
+	g_assert(command->has_id == test->has_id);
+	if (test->has_id)
+		g_assert(command->id == test->id);
+}
+
 struct display_text_test {
 	const unsigned char *pdu;
 	unsigned int pdu_len;
-	const char *expected;
 	unsigned char qualifier;
-	unsigned char icon_qualifier;
-	unsigned char icon_id;
-	enum stk_duration_type duration_unit;
-	unsigned char duration_interval;
+	const char *text;
+	struct stk_icon_id icon_id;
+	ofono_bool_t immediate_response;
+	struct stk_duration duration;
+	struct stk_text_attribute text_attribute;
+	struct stk_frame_id frame_id;
 };
 
 unsigned char display_text_111[] = { 0xD0, 0x1A, 0x81, 0x03, 0x01, 0x21, 0x80,
@@ -151,103 +208,111 @@ unsigned char display_text_1011[] = { 0xD0, 0x12, 0x81, 0x03, 0x01, 0x21, 0x80,
 static struct display_text_test display_text_data_111 = {
 	.pdu = display_text_111,
 	.pdu_len = sizeof(display_text_111),
-	.expected = "Toolkit Test 1",
-	.qualifier = 0x80
+	.qualifier = 0x80,
+	.text = "Toolkit Test 1"
 };
 
 static struct display_text_test display_text_data_131 = {
 	.pdu = display_text_131,
 	.pdu_len = sizeof(display_text_131),
-	.expected = "Toolkit Test 2",
-	.qualifier = 0x81
+	.qualifier = 0x81,
+	.text = "Toolkit Test 2"
 };
 
 static struct display_text_test display_text_data_141 = {
 	.pdu = display_text_141,
 	.pdu_len = sizeof(display_text_141),
-	.expected = "Toolkit Test 3",
-	.qualifier = 0x80
+	.qualifier = 0x80,
+	.text = "Toolkit Test 3"
 };
 
 static struct display_text_test display_text_data_151 = {
 	.pdu = display_text_151,
 	.pdu_len = sizeof(display_text_151),
-	.expected = "Toolkit Test 4",
-	.qualifier = 0x00
+	.qualifier = 0x00,
+	.text = "Toolkit Test 4"
 };
 
 static struct display_text_test display_text_data_161 = {
 	.pdu = display_text_161,
 	.pdu_len = sizeof(display_text_161),
-	.expected = "This command instructs the ME to display a text message. "
+	.qualifier = 0x80,
+	.text = "This command instructs the ME to display a text message. "
 			"It allows the SIM to define the priority of that "
 			"message, and the text string format. Two types of "
-			"prio",
-	.qualifier = 0x80
+			"prio"
 };
 
 static struct display_text_test display_text_data_171 = {
 	.pdu = display_text_171,
 	.pdu_len = sizeof(display_text_171),
-	.expected = "<GO-BACKWARDS>",
-	.qualifier = 0x80
+	.qualifier = 0x80,
+	.text = "<GO-BACKWARDS>"
 };
 
 static struct display_text_test display_text_data_511 = {
 	.pdu = display_text_511,
 	.pdu_len = sizeof(display_text_511),
-	.expected = "Basic Icon",
 	.qualifier = 0x80,
-	.icon_id = 0x01,
-	.icon_qualifier = 0x00,
+	.text = "Basic Icon",
+	.icon_id = {
+		.qualifier = 0x00,
+		.id = 0x01
+	}
 };
 
 static struct display_text_test display_text_data_521 = {
 	.pdu = display_text_521,
 	.pdu_len = sizeof(display_text_521),
-	.expected = "Colour Icon",
 	.qualifier = 0x80,
-	.icon_id = 0x02,
-	.icon_qualifier = 0x00,
+	.text = "Colour Icon",
+	.icon_id = {
+		.qualifier = 0x00,
+		.id = 0x02
+	}
 };
 
 static struct display_text_test display_text_data_531 = {
 	.pdu = display_text_531,
 	.pdu_len = sizeof(display_text_531),
-	.expected = "Basic Icon",
 	.qualifier = 0x80,
-	.icon_id = 0x01,
-	.icon_qualifier = 0x01,
+	.text = "Basic Icon",
+	.icon_id = {
+		.qualifier = 0x01,
+		.id = 0x01
+	}
 };
 
 static struct display_text_test display_text_data_611 = {
 	.pdu = display_text_611,
 	.pdu_len = sizeof(display_text_611),
-	.expected = "ЗДРАВСТВУЙТЕ",
-	.qualifier = 0x80
+	.qualifier = 0x80,
+	.text = "ЗДРАВСТВУЙТЕ"
 };
 
 static struct display_text_test display_text_data_711 = {
 	.pdu = display_text_711,
 	.pdu_len = sizeof(display_text_711),
-	.expected = "10 Second",
 	.qualifier = 0x80,
-	.duration_unit = STK_DURATION_TYPE_SECONDS,
-	.duration_interval = 10,
+	.text = "10 Second",
+	.duration = {
+		.unit = STK_DURATION_TYPE_SECONDS,
+		.interval = 10,
+	}
 };
 
 static struct display_text_test display_text_data_911 = {
 	.pdu = display_text_911,
 	.pdu_len = sizeof(display_text_911),
-	.expected = "你好",
-	.qualifier = 0x80
+	.qualifier = 0x80,
+	.text = "你好"
 };
 
 static struct display_text_test display_text_data_1011 = {
 	.pdu = display_text_1011,
 	.pdu_len = sizeof(display_text_1011),
-	.expected = "80ル",
-	.qualifier = 0x80
+	.qualifier = 0x80,
+	.text = "80ル"
 };
 
 /* Defined in TS 102.384 Section 27.22.4.1 */
@@ -268,21 +333,14 @@ static void test_display_text(gconstpointer data)
 	g_assert(command->dst == STK_DEVICE_IDENTITY_TYPE_DISPLAY);
 
 	g_assert(command->display_text.text);
-
-	g_assert(g_str_equal(test->expected, command->display_text.text));
-
-	if (test->icon_id > 0) {
-		g_assert(command->display_text.icon_id.id == test->icon_id);
-		g_assert(command->display_text.icon_id.qualifier ==
-				test->icon_qualifier);
-	}
-
-	if (test->duration_interval > 0) {
-		g_assert(command->display_text.duration.unit ==
-				test->duration_unit);
-		g_assert(command->display_text.duration.interval ==
-				test->duration_interval);
-	}
+	check_text(command->display_text.text, test->text);
+	check_icon_id(&command->display_text.icon_id, &test->icon_id);
+	check_common_bool(command->display_text.immediate_response,
+						test->immediate_response);
+	check_duration(&command->display_text.duration, &test->duration);
+	check_text_attr(&command->display_text.text_attribute,
+						&test->text_attribute);
+	check_frame_id(&command->display_text.frame_id, &test->frame_id);
 
 	stk_command_free(command);
 }
Signature · 1 line
1.7.0.4
author
#8 lore raw

PATCH 5/6 stkutil: Add parser for more time proactive commands

In reply to Yang Gu
---
 src/stkutil.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
src/stkutil.c +15 -0
index 2b43e85..ca0ee26 100644
@@ -2193,6 +2193,18 @@ static gboolean parse_get_input(struct stk_command *command,
 	return TRUE;
 }
 
+static gboolean parse_more_time(struct stk_command *command,
+					struct comprehension_tlv_iter *iter)
+{
+	if (command->src != STK_DEVICE_IDENTITY_TYPE_UICC)
+		return FALSE;
+
+	if (command->dst != STK_DEVICE_IDENTITY_TYPE_TERMINAL)
+		return FALSE;
+
+	return TRUE;
+}
+
 static void destroy_send_sms(struct stk_command *command)
 {
 	g_free(command->send_sms.alpha_id);
@@ -2310,6 +2322,9 @@ struct stk_command *stk_command_new_from_pdu(const unsigned char *pdu,
 	case STK_COMMAND_TYPE_GET_INPUT:
 		ok = parse_get_input(command, &iter);
 		break;
+	case STK_COMMAND_TYPE_MORE_TIME:
+		ok = parse_more_time(command, &iter);
+		break;
 	case STK_COMMAND_TYPE_SEND_SMS:
 		ok = parse_send_sms(command, &iter);
 		break;
Signature · 1 line
1.7.0.4
author
#9 lore raw

PATCH 6/6 test-stkutil: Add unit test for more time parser

In reply to Yang Gu
---
 unit/test-stkutil.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
unit/test-stkutil.c +38 -0
index 0961368..427d7a2 100644
@@ -405,6 +405,41 @@ static void test_get_input(gconstpointer data)
 	stk_command_free(command);
 }
 
+struct more_time_test {
+	const unsigned char *pdu;
+	unsigned int pdu_len;
+	unsigned char qualifier;
+};
+
+static unsigned char more_time_111[] = { 0xD0, 0x09, 0x81, 0x03, 0x01, 0x02,
+						0x00, 0x82, 0x02, 0x81, 0x82 };
+
+static struct more_time_test more_time_data_111 = {
+	.pdu = more_time_111,
+	.pdu_len = sizeof(more_time_111),
+	.qualifier = 0x00,
+};
+
+/* Defined in TS 102.384 Section 27.22.4.4 */
+static void test_more_time(gconstpointer data)
+{
+	const struct get_input_test *test = data;
+	struct stk_command *command;
+
+	command = stk_command_new_from_pdu(test->pdu, test->pdu_len);
+
+	g_assert(command);
+
+	g_assert(command->number == 1);
+	g_assert(command->type == STK_COMMAND_TYPE_MORE_TIME);
+	g_assert(command->qualifier == test->qualifier);
+
+	g_assert(command->src == STK_DEVICE_IDENTITY_TYPE_UICC);
+	g_assert(command->dst == STK_DEVICE_IDENTITY_TYPE_TERMINAL);
+
+	stk_command_free(command);
+}
+
 struct send_sms_test {
 	const unsigned char *pdu;
 	unsigned int pdu_len;
@@ -516,6 +551,9 @@ int main(int argc, char **argv)
 	g_test_add_data_func("/teststk/Get Input 1.1.1",
 				&get_input_data_111, test_get_input);
 
+	g_test_add_data_func("/teststk/More Time 1.1.1",
+				&more_time_data_111, test_more_time);
+
 	g_test_add_data_func("/teststk/Send SMS 1.1",
 				&send_sms_data_11, test_send_sms);
 
Signature · 1 line
1.7.0.4
#10 lore raw
In reply to Yang Gu

Hi Yang,

Quoted 3 lines
---
 src/stkutil.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Good catch, patch has been applied. Thanks.

Regards,
-Denis