[PATCH 1/4] Fix the structure of stk_subaddress

[PATCH 1/4] Fix the structure of stk_subaddress

[Yang Gu]

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

---
 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

[PATCH 2/4] Handle the conversion failure when parsing item

[Yang Gu]

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

---
 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

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

[Yang Gu]

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

---
 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

[PATCH 4/4] Add parser for location information objects

[Yang Gu]

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

---
 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

Re: [PATCH 2/4] Handle the conversion failure when parsing item

[Marcel Holtmann]

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

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

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

[Marcel Holtmann]

[-- 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

Re: [PATCH 4/4] Add parser for location information objects

[Marcel Holtmann]

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

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

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

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3468 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;
> +		}
> +	}
> +

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
> 

Re: [PATCH 2/4] Handle the conversion failure when parsing item

[Denis Kenzior]

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

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

Re: [PATCH 1/4] Fix the structure of stk_subaddress

[Denis Kenzior]

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

Hi Yang,

> ---
>  src/stkutil.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 2/4] Handle the conversion failure when parsing item

[Denis Kenzior]

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

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

RE: [PATCH 2/4] Handle the conversion failure when parsing item

[Gu, Yang]

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

Hi Denis,

>-----Original Message-----
>From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)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

Re: [PATCH 2/4] Handle the conversion failure when parsing item

[Marcel Holtmann]

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

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

RE: [PATCH 2/4] Handle the conversion failure when parsing item

[Marcel Holtmann]

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

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