9:01 p.m.
---
src/stkutil.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/stkutil.h b/src/stkutil.h
index b59c9f0..b408f38 100644
--- a/src/stkutil.h
+++ b/src/stkutil.h
@@ -286,7 +286,7 @@ struct stk_address {
*/
struct stk_subaddress {
unsigned char len;
- unsigned char *subaddr[23];
+ unsigned char subaddr[23];
};
/*
--
1.6.3.3
9:01 p.m.
New subject: [PATCH 2/4] Handle the conversion failure when parsing item
---
src/stkutil.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/src/stkutil.c b/src/stkutil.c
index 787f7eb..ceba2d5 100644
--- a/src/stkutil.c
+++ b/src/stkutil.c
@@ -190,6 +190,7 @@ static gboolean parse_dataobj_item(struct comprehension_tlv_iter
*iter,
struct stk_item *item = user;
const unsigned char *data;
unsigned int len;
+ char *utf8;
if (comprehension_tlv_iter_get_tag(iter) !=
STK_DATA_OBJECT_TYPE_ITEM)
@@ -206,7 +207,12 @@ static gboolean parse_dataobj_item(struct comprehension_tlv_iter
*iter,
return FALSE;
item->id = data[0];
- item->text = sim_string_to_utf8(data+1, len-1);
+ utf8 = sim_string_to_utf8(data+1, len-1);
+
+ if (utf8 == NULL)
+ return FALSE;
+
+ item->text = utf8;
return TRUE;
}
--
1.6.3.3
9:01 p.m.
New subject: [PATCH 3/4] Add parser for file list objects
---
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;
+ }
+ }
+
+ *fl = g_slist_reverse(*fl);
+
+ return TRUE;
+}
+
/* Defined in TS 102.223 Section 8.31 */
static gboolean parse_dataobj_icon_id(struct comprehension_tlv_iter *iter,
void *user)
@@ -523,6 +563,8 @@ static dataobj_handler handler_for_type(enum stk_data_object_type
type)
return parse_dataobj_text;
case STK_DATA_OBJECT_TYPE_TONE:
return parse_dataobj_tone;
+ case STK_DATA_OBJECT_TYPE_FILE_LIST:
+ return parse_dataobj_file_list;
case STK_DATA_OBJECT_TYPE_ICON_ID:
return parse_dataobj_icon_id;
case STK_DATA_OBJECT_TYPE_IMMEDIATE_RESPONSE:
diff --git a/src/stkutil.h b/src/stkutil.h
index b408f38..5b1a44b 100644
--- a/src/stkutil.h
+++ b/src/stkutil.h
@@ -345,6 +345,12 @@ struct stk_result {
unsigned char *additional;
};
+/* Define the struct of single file in TS102.223 Section 8.18 */
+struct stk_file {
+ unsigned char *file;
+ unsigned int len;
+};
+
/*
* According to 102.223 Section 8.72 the length of text attribute CTLV is 1
* byte. This means that the maximum size is 127 according to the rules
--
1.6.3.3
9:01 p.m.
New subject: [PATCH 4/4] Add parser for location information objects
---
src/simutil.c | 2 +-
src/simutil.h | 1 +
src/stkutil.c | 40 +++++++++++++++++++++++++++++++++++++---
src/stkutil.h | 9 +++++++++
4 files changed, 48 insertions(+), 4 deletions(-)
diff --git a/src/simutil.c b/src/simutil.c
index d9383b7..65ffa36 100644
--- a/src/simutil.c
+++ b/src/simutil.c
@@ -538,7 +538,7 @@ static char *sim_network_name_parse(const unsigned char *buffer, int
length,
return ret;
}
-static void parse_mcc_mnc(const guint8 *bcd, char *mcc, char *mnc)
+void parse_mcc_mnc(const guint8 *bcd, char *mcc, char *mnc)
{
static const char digit_lut[] = "0123456789*#abd\0";
guint8 digit;
diff --git a/src/simutil.h b/src/simutil.h
index 043c21f..09964a8 100644
--- a/src/simutil.h
+++ b/src/simutil.h
@@ -181,6 +181,7 @@ const struct sim_eons_operator_info *sim_eons_lookup(struct sim_eons
*eons,
const char *mnc);
void sim_eons_free(struct sim_eons *eons);
+void parse_mcc_mnc(const guint8 *bcd, char *mcc, char *mnc);
struct sim_spdi *sim_spdi_new(const guint8 *tlv, int length);
gboolean sim_spdi_lookup(struct sim_spdi *spdi,
const char *mcc, const char *mnc);
diff --git a/src/stkutil.c b/src/stkutil.c
index 9f3bc0b..9fa7705 100644
--- a/src/stkutil.c
+++ b/src/stkutil.c
@@ -413,7 +413,7 @@ static gboolean parse_dataobj_file_list(struct comprehension_tlv_iter
*iter,
GSList **fl = user;
const unsigned char *data;
unsigned int len;
- unsigned int i;
+ unsigned int i = 1;
unsigned int start = 1;
struct stk_file *sf;
@@ -430,8 +430,10 @@ static gboolean parse_dataobj_file_list(struct comprehension_tlv_iter
*iter,
if (data[start] != 0x3f)
return FALSE;
- for (i = start + 4; i <= len; i += 2) {
- if ((data[i] == 0x3f) || (i == len)) {
+ for (i = start + 4; i < len; i += 2) {
+ if ((data[i] == 0x3f) || (i+2 >= len)) {
+ if (i + 2 >= len)
+ i += 2;
sf = g_new0(struct stk_file, 1);
sf->file = g_malloc(i-start);
memcpy(sf->file, data+start, i-start);
@@ -446,6 +448,36 @@ static gboolean parse_dataobj_file_list(struct comprehension_tlv_iter
*iter,
return TRUE;
}
+/* Defined in TS 102.223 Section 8.19 */
+static gboolean parse_dataobj_location_info(
+ struct comprehension_tlv_iter *iter, void *user)
+{
+ struct stk_location_info *li = user;
+ const unsigned char *data;
+ unsigned int len;
+
+ if (comprehension_tlv_iter_get_tag(iter) !=
+ STK_DATA_OBJECT_TYPE_LOCATION_INFO)
+ return FALSE;
+
+ len = comprehension_tlv_iter_get_length(iter);
+ if ((len != 5) && (len != 7) && (len != 9))
+ return FALSE;
+
+ data = comprehension_tlv_iter_get_data(iter);
+
+ parse_mcc_mnc(data, li->mcc, li->mnc);
+ li->lac_tac = data[3] << 8 | data[4];
+
+ if (len >= 7)
+ memcpy(li->cell_id, data+5, 2);
+
+ if (len == 9)
+ memcpy(li->ext_cell_id, data+7, 2);
+
+ return TRUE;
+}
+
/* Defined in TS 102.223 Section 8.31 */
static gboolean parse_dataobj_icon_id(struct comprehension_tlv_iter *iter,
void *user)
@@ -565,6 +597,8 @@ static dataobj_handler handler_for_type(enum stk_data_object_type
type)
return parse_dataobj_tone;
case STK_DATA_OBJECT_TYPE_FILE_LIST:
return parse_dataobj_file_list;
+ case STK_DATA_OBJECT_TYPE_LOCATION_INFO:
+ return parse_dataobj_location_info;
case STK_DATA_OBJECT_TYPE_ICON_ID:
return parse_dataobj_icon_id;
case STK_DATA_OBJECT_TYPE_IMMEDIATE_RESPONSE:
diff --git a/src/stkutil.h b/src/stkutil.h
index 5b1a44b..def2555 100644
--- a/src/stkutil.h
+++ b/src/stkutil.h
@@ -351,6 +351,15 @@ struct stk_file {
unsigned int len;
};
+/* Defined in TS 102.223 Section 8.19 */
+struct stk_location_info {
+ char mnc[OFONO_MAX_MNC_LENGTH + 1];
+ char mcc[OFONO_MAX_MCC_LENGTH + 1];
+ guint16 lac_tac;
+ char cell_id[2];
+ char ext_cell_id[2];
+};
+
/*
* According to 102.223 Section 8.72 the length of text attribute CTLV is 1
* byte. This means that the maximum size is 127 according to the rules
--
1.6.3.3
5:36 a.m.
New subject: [PATCH 2/4] Handle the conversion failure when parsing item
Hi Yang,
src/stkutil.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/src/stkutil.c b/src/stkutil.c
index 787f7eb..ceba2d5 100644
--- a/src/stkutil.c
+++ b/src/stkutil.c
@@ -190,6 +190,7 @@ static gboolean parse_dataobj_item(struct comprehension_tlv_iter
*iter,
struct stk_item *item = user;
const unsigned char *data;
unsigned int len;
+ char *utf8;
if (comprehension_tlv_iter_get_tag(iter) !=
STK_DATA_OBJECT_TYPE_ITEM)
@@ -206,7 +207,12 @@ static gboolean parse_dataobj_item(struct comprehension_tlv_iter
*iter,
return FALSE;
item->id = data[0];
- item->text = sim_string_to_utf8(data+1, len-1);
+ utf8 = sim_string_to_utf8(data+1, len-1);
please add spaces between operators like +. So data + 1, len - 1.
+ if (utf8 == NULL)
+ return FALSE;
+
+ item->text = utf8;
Why bother with utf8 variable? Just do
if (item->text == NULL)
return FALSE;
Regards
Marcel
5:44 a.m.
New subject: [PATCH 3/4] Add parser for file list objects
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
5:46 a.m.
New subject: [PATCH 4/4] Add parser for location information objects
Hi Yang,
src/simutil.c | 2 +-
src/simutil.h | 1 +
src/stkutil.c | 40 +++++++++++++++++++++++++++++++++++++---
src/stkutil.h | 9 +++++++++
4 files changed, 48 insertions(+), 4 deletions(-)
diff --git a/src/simutil.c b/src/simutil.c
index d9383b7..65ffa36 100644
--- a/src/simutil.c
+++ b/src/simutil.c
@@ -538,7 +538,7 @@ static char *sim_network_name_parse(const unsigned char *buffer, int
length,
return ret;
}
-static void parse_mcc_mnc(const guint8 *bcd, char *mcc, char *mnc)
+void parse_mcc_mnc(const guint8 *bcd, char *mcc, char *mnc)
{
static const char digit_lut[] = "0123456789*#abd\0";
guint8 digit;
diff --git a/src/simutil.h b/src/simutil.h
index 043c21f..09964a8 100644
--- a/src/simutil.h
+++ b/src/simutil.h
@@ -181,6 +181,7 @@ const struct sim_eons_operator_info *sim_eons_lookup(struct sim_eons
*eons,
const char *mnc);
void sim_eons_free(struct sim_eons *eons);
+void parse_mcc_mnc(const guint8 *bcd, char *mcc, char *mnc);
struct sim_spdi *sim_spdi_new(const guint8 *tlv, int length);
gboolean sim_spdi_lookup(struct sim_spdi *spdi,
const char *mcc, const char *mnc);
diff --git a/src/stkutil.c b/src/stkutil.c
index 9f3bc0b..9fa7705 100644
--- a/src/stkutil.c
+++ b/src/stkutil.c
@@ -413,7 +413,7 @@ static gboolean parse_dataobj_file_list(struct comprehension_tlv_iter
*iter,
GSList **fl = user;
const unsigned char *data;
unsigned int len;
- unsigned int i;
+ unsigned int i = 1;
unsigned int start = 1;
struct stk_file *sf;
this change makes no sense to me. Please only initialize variables when
really needed. I really want the compiler to warn us when we use
variables unexpectedly.
Regards
Marcel
7:01 a.m.
New subject: [PATCH 3/4] Add parser for file list objects
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;
+ }
+ }
+
Ok, so the logic here actually makes no sense. Please review TS 31.124 for an
example on how this actually works. Basically these guys are relying on
0x3FXX as the marker for identifying the Master File and then relying on the
type of MF/DF/EF to figure out where the file identifier ends. Completely
insane.
For reference from TS 11.11:
The first byte identifies the type of file, and for GSM is:
‑ '3F': Master File;
‑ '7F': 1st level Dedicated File;
- '5F': 2nd level Dedicated File;
‑ '2F': Elementary File under the Master File;
‑ '6F': Elementary File under a 1st level Dedicated File;
- '4F': Elementary File under 2nd level Dedicated File.
You will have to walk each first byte to determine when the file actually ends.
I suggest allocating a maximum of 8 bytes to the file structure (2 bytes for
MF, 2 bytes for 1st level DF, 2 bytes for 2nd level DF and 2 bytes for EF)
Regards,
-Denis
+ *fl = g_slist_reverse(*fl);
+
+ return TRUE;
+}
+
/* Defined in TS 102.223 Section 8.31 */
static gboolean parse_dataobj_icon_id(struct comprehension_tlv_iter *iter,
void *user)
@@ -523,6 +563,8 @@ static dataobj_handler handler_for_type(enum
stk_data_object_type type) return parse_dataobj_text;
case STK_DATA_OBJECT_TYPE_TONE:
return parse_dataobj_tone;
+ case STK_DATA_OBJECT_TYPE_FILE_LIST:
+ return parse_dataobj_file_list;
case STK_DATA_OBJECT_TYPE_ICON_ID:
return parse_dataobj_icon_id;
case STK_DATA_OBJECT_TYPE_IMMEDIATE_RESPONSE:
diff --git a/src/stkutil.h b/src/stkutil.h
index b408f38..5b1a44b 100644
--- a/src/stkutil.h
+++ b/src/stkutil.h
@@ -345,6 +345,12 @@ struct stk_result {
unsigned char *additional;
};
+/* Define the struct of single file in TS102.223 Section 8.18 */
+struct stk_file {
+ unsigned char *file;
+ unsigned int len;
+};
+
/*
* According to 102.223 Section 8.72 the length of text attribute CTLV is
1 * byte. This means that the maximum size is 127 according to the rules
7:27 a.m.
New subject: [PATCH 2/4] Handle the conversion failure when parsing item
Hi Marcel,
> + if (utf8 == NULL)
> + return FALSE;
> +
> + item->text = utf8;
Why bother with utf8 variable? Just do
if (item->text == NULL)
return FALSE;
I actually find this acceptable because 'item' is a return structure, so we
should avoid modifying it in case of an error.
Regards,
-Denis
7:29 a.m.
New subject: [PATCH 2/4] Handle the conversion failure when parsing item
Hi Yang,
- item->text = sim_string_to_utf8(data+1, len-1);
+ utf8 = sim_string_to_utf8(data+1, len-1);
I applied the patch and fixed up the arithmetic operator style issue. Please
make sure you follow this convention from now on.
Regards,
-Denis
2:33 p.m.
New subject: [PATCH 2/4] Handle the conversion failure when parsing item
Hi Denis,
-----Original Message-----
From: ofono-bounces(a)ofono.org [mailto:ofono-bounces@ofono.org] On Behalf Of
Denis Kenzior
Sent: Wednesday, March 17, 2010 4:30 AM
To: ofono(a)ofono.org
Subject: Re: [PATCH 2/4] Handle the conversion failure when parsing item
Hi Yang,
> - item->text = sim_string_to_utf8(data+1, len-1);
> + utf8 = sim_string_to_utf8(data+1, len-1);
I applied the patch and fixed up the arithmetic operator style issue. Please
make sure you follow this convention from now on.
Thank you, and I will pay attention to this in future.
I'm always wondering what's the complete coding style we should follow. Can we
have some document to describe them? I have collected some of them:
1. Check the patch using checkpatch.pl (checkpatch.pl --no-tree patch_name). In theory,
you need to clean up all the warnings and errors except one "ERROR: Missing
Signed-off-by: line(s)". However, sometimes, the warning of exceeding maximum
characters for a line (80 characters) can be ignored.
2. There should be a blank line before if statement, unless it is nested and not preceded
by an expression or variable declaration.
3. There should be space before and after operator.
4. It's better not to have a complicated statement for if. You may judge its contrary
condition and return | break | continue ASAP.
5. Better to use abbreviation, rather than full name, to name a variable, function,
struct, etc.
Any others?
Regards,
-Denis
_______________________________________________
ofono mailing list
ofono(a)ofono.org
http://lists.ofono.org/listinfo/ofono
Regards,
-Yang
3:40 p.m.
New subject: [PATCH 2/4] Handle the conversion failure when parsing item
Hi Denis,
> > + if (utf8 == NULL)
> > + return FALSE;
> > +
> > + item->text = utf8;
>
> Why bother with utf8 variable? Just do
>
> if (item->text == NULL)
> return FALSE;
>
I actually find this acceptable because 'item' is a return structure, so we
should avoid modifying it in case of an error.
in general it makes no difference since the assigned pointer will be the
same as the previous one. However this is a minor nitpick and nothing
major. So I am fine either way.
Regards
Marcel
3:44 p.m.
New subject: [PATCH 2/4] Handle the conversion failure when parsing item
Hi Yang,
>> - item->text = sim_string_to_utf8(data+1, len-1);
>> + utf8 = sim_string_to_utf8(data+1, len-1);
>
>I applied the patch and fixed up the arithmetic operator style issue. Please
>make sure you follow this convention from now on.
Thank you, and I will pay attention to this in future.
I'm always wondering what's the complete coding style we should follow. Can we
have some document to describe them? I have collected some of them:
1. Check the patch using checkpatch.pl (checkpatch.pl --no-tree patch_name). In theory,
you need to clean up all the warnings and errors except one "ERROR: Missing
Signed-off-by: line(s)". However, sometimes, the warning of exceeding maximum
characters for a line (80 characters) can be ignored.
2. There should be a blank line before if statement, unless it is nested and not preceded
by an expression or variable declaration.
3. There should be space before and after operator.
4. It's better not to have a complicated statement for if. You may judge its contrary
condition and return | break | continue ASAP.
5. Better to use abbreviation, rather than full name, to name a variable, function,
struct, etc.
Any others?
general rule is to look at the already existing code. We might need
checkpatch.pl specific for BlueZ, ConnMan and oFono. There are cases
where some of the style is different from kernel code. However it is not
that major.
We all make mistakes and sometimes think to complicated. That is why we
do reviews and enforce the coding style. Just keep an eye on the simple
mistakes like the one above. They are pretty obvious ;)
Regards
Marcel
4568
days inactive
4569
days old
13 comments
4 participants
participants (4)
-
Denis Kenzior -
Gu, Yang -
Marcel Holtmann -
Yang Gu