[PATCH] atmodem/phonebook: parse text as hexstring

[PATCH] atmodem/phonebook: parse text as hexstring

[Thadeu Lima de Souza Cascardo]

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

Some modems omit the quotes when dumping strings in UCS2. Parsing
them as hexstring already does the hex decoding and accepts missing
quotes.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 drivers/atmodem/phonebook.c |  139 ++++++++++++++++++++----------------------
 1 files changed, 66 insertions(+), 73 deletions(-)

diff --git a/drivers/atmodem/phonebook.c b/drivers/atmodem/phonebook.c
index 5e7a52b..59fcbbc 100644
--- a/drivers/atmodem/phonebook.c
+++ b/drivers/atmodem/phonebook.c
@@ -59,16 +59,50 @@ struct pb_data {
 	GAtChat *chat;
 };
 
-static char *ucs2_to_utf8(const char *str)
+static void warn_bad_text(const char *text)
 {
-	long len;
-	unsigned char *ucs2;
+	ofono_warn("Name field conversion to UTF8"
+		" failed, this can indicate a"
+		" problem with modem"
+		" integration, as this field"
+		" is required by 27.007."
+		"  Contents of name reported"
+		" by modem: %s", text);
+}
+
+static gboolean parse_text(GAtResultIter *iter, char **str, int encoding)
+{
+	const char *string;
+	const guint8 *hex;
+	int len;
 	char *utf8;
-	ucs2 = decode_hex(str, -1, &len, 0);
-	utf8 = g_convert((char *)ucs2, len, "UTF-8//TRANSLIT", "UCS-2BE",
-					NULL, NULL, NULL);
-	g_free(ucs2);
-	return utf8;
+	/* charset_current is either CHARSET_UCS2 or CHARSET_UTF8 */
+	if (encoding == CHARSET_UCS2) {
+		/* Some devices ommit the quotes, so use hexstring,
+		 * which also decode to hex */
+		if (g_at_result_iter_next_hexstring(iter, &hex, &len))
+			utf8 = g_convert((const gchar *)hex, len,
+						"UTF-8//TRANSLIT", "UCS-2BE",
+						NULL, NULL, NULL);
+		else
+			return FALSE;
+		if (utf8) {
+			*str = utf8;
+			return TRUE;
+		} else {
+			warn_bad_text(string);
+			return FALSE;
+		}
+	} else {
+		/* In the case of IRA charset, assume these are Latin1
+		 * characters, same as in UTF8
+		 */
+		if (g_at_result_iter_next_string(iter, &string)) {
+				*str = g_strdup(string);
+				return TRUE;
+		}
+	}
+	return FALSE;
 }
 
 static const char *best_charset(int supported)
@@ -110,15 +144,15 @@ static void at_cpbr_notify(GAtResult *result, gpointer user_data)
 		int index;
 		const char *number;
 		int type;
-		const char *text;
+		char *text;
 		int hidden = -1;
-		const char *group = NULL;
+		char *group = NULL;
 		const char *adnumber = NULL;
 		int adtype = -1;
-		const char *secondtext = NULL;
-		const char *email = NULL;
-		const char *sip_uri = NULL;
-		const char *tel_uri = NULL;
+		char *secondtext = NULL;
+		char *email = NULL;
+		char *sip_uri = NULL;
+		char *tel_uri = NULL;
 
 		if (!g_at_result_iter_next_number(&iter, &index))
 			continue;
@@ -129,70 +163,29 @@ static void at_cpbr_notify(GAtResult *result, gpointer user_data)
 		if (!g_at_result_iter_next_number(&iter, &type))
 			continue;
 
-		if (!g_at_result_iter_next_string(&iter, &text))
+		if (!parse_text(&iter, &text, current))
 			continue;
 
 		g_at_result_iter_next_number(&iter, &hidden);
-		g_at_result_iter_next_string(&iter, &group);
+		parse_text(&iter, &group, current);
 		g_at_result_iter_next_string(&iter, &adnumber);
 		g_at_result_iter_next_number(&iter, &adtype);
-		g_at_result_iter_next_string(&iter, &secondtext);
-		g_at_result_iter_next_string(&iter, &email);
-		g_at_result_iter_next_string(&iter, &sip_uri);
-		g_at_result_iter_next_string(&iter, &tel_uri);
-
-		/* charset_current is either CHARSET_UCS2 or CHARSET_UTF8 */
-		if (current == CHARSET_UCS2) {
-			char *text_utf8;
-			char *group_utf8 = NULL;
-			char *secondtext_utf8 = NULL;
-			char *email_utf8 = NULL;
-			char *sip_uri_utf8 = NULL;
-			char *tel_uri_utf8 = NULL;
-
-			text_utf8 = ucs2_to_utf8(text);
-
-			if (text_utf8 == NULL)
-				ofono_warn("Name field conversion to UTF8"
-						" failed, this can indicate a"
-						" problem with modem"
-						" integration, as this field"
-						" is required by 27.007."
-						"  Contents of name reported"
-						" by modem: %s", text);
-
-			if (group)
-				group_utf8 = ucs2_to_utf8(group);
-			if (secondtext)
-				secondtext_utf8 = ucs2_to_utf8(secondtext);
-			if (email)
-				email_utf8 = ucs2_to_utf8(email);
-			if (sip_uri)
-				sip_uri_utf8 = ucs2_to_utf8(sip_uri);
-			if (tel_uri)
-				tel_uri_utf8 = ucs2_to_utf8(tel_uri);
-
-			ofono_phonebook_entry(pb, index, number, type,
-				text_utf8, hidden, group_utf8, adnumber,
-				adtype, secondtext_utf8, email_utf8,
-				sip_uri_utf8, tel_uri_utf8);
-
-			g_free(text_utf8);
-			g_free(group_utf8);
-			g_free(secondtext_utf8);
-			g_free(email_utf8);
-			g_free(sip_uri_utf8);
-			g_free(tel_uri_utf8);
-		} else {
-			/* In the case of IRA charset, assume these are Latin1
-			 * characters, same as in UTF8
-			 */
-			ofono_phonebook_entry(pb, index, number, type,
-				text, hidden, group, adnumber,
-				adtype, secondtext, email,
-				sip_uri, tel_uri);
-
-		}
+		parse_text(&iter, &secondtext, current);
+		parse_text(&iter, &email, current);
+		parse_text(&iter, &sip_uri, current);
+		parse_text(&iter, &tel_uri, current);
+
+		ofono_phonebook_entry(pb, index, number, type,
+			text, hidden, group, adnumber,
+			adtype, secondtext, email,
+			sip_uri, tel_uri);
+
+		g_free(text);
+		g_free(group);
+		g_free(secondtext);
+		g_free(email);
+		g_free(sip_uri);
+		g_free(tel_uri);
 	}
 }
 
-- 
1.7.1

RE: [PATCH] atmodem/phonebook: parse text as hexstring

[Gu, Yang]

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

Hi, 


>-----Original Message-----
>From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)ofono.org] On Behalf Of
>Thadeu Lima de Souza Cascardo
>Sent: Monday, August 09, 2010 6:28 AM
>To: ofono(a)ofono.org
>Subject: [PATCH] atmodem/phonebook: parse text as hexstring
>
>Some modems omit the quotes when dumping strings in UCS2. Parsing
>them as hexstring already does the hex decoding and accepts missing
>quotes.
>
>Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
>---
> drivers/atmodem/phonebook.c |  139 ++++++++++++++++++++----------------------
> 1 files changed, 66 insertions(+), 73 deletions(-)
>
>diff --git a/drivers/atmodem/phonebook.c b/drivers/atmodem/phonebook.c
>index 5e7a52b..59fcbbc 100644
>--- a/drivers/atmodem/phonebook.c
>+++ b/drivers/atmodem/phonebook.c
>@@ -59,16 +59,50 @@ struct pb_data {
> 	GAtChat *chat;
> };
>
>-static char *ucs2_to_utf8(const char *str)
>+static void warn_bad_text(const char *text)
> {
>-	long len;
>-	unsigned char *ucs2;
>+	ofono_warn("Name field conversion to UTF8"
>+		" failed, this can indicate a"
>+		" problem with modem"
>+		" integration, as this field"
>+		" is required by 27.007."
>+		"  Contents of name reported"
>+		" by modem: %s", text);
>+}

Why this warning is extracted as a function here?

>+
>+static gboolean parse_text(GAtResultIter *iter, char **str, int encoding)
>+{
>+	const char *string;
>+	const guint8 *hex;
>+	int len;
> 	char *utf8;
>-	ucs2 = decode_hex(str, -1, &len, 0);
>-	utf8 = g_convert((char *)ucs2, len, "UTF-8//TRANSLIT", "UCS-2BE",
>-					NULL, NULL, NULL);
>-	g_free(ucs2);
>-	return utf8;
>+	/* charset_current is either CHARSET_UCS2 or CHARSET_UTF8 */

Please help to add CHARSET_IRA here. I know the original code is not correct.

>+	if (encoding == CHARSET_UCS2) {
>+		/* Some devices ommit the quotes, so use hexstring,
>+		 * which also decode to hex */

Misspell the "omit" here. We have guideline to write multiple line comments, please refer doc/coding_style.txt and section M2.

>+		if (g_at_result_iter_next_hexstring(iter, &hex, &len))
>+			utf8 = g_convert((const gchar *)hex, len,
>+						"UTF-8//TRANSLIT", "UCS-2BE",
>+						NULL, NULL, NULL);
>+		else
>+			return FALSE;

Please insert a blank line here.

>+		if (utf8) {
>+			*str = utf8;
>+			return TRUE;
>+		} else {
>+			warn_bad_text(string);

We only report the warning when the field "name" (i.e., text) couldn't be converted correctly, for it's a mandatory field. Otherwise, we don't issue the warning.

>+			return FALSE;
>+		}
>+	} else {
>+		/* In the case of IRA charset, assume these are Latin1
>+		 * characters, same as in UTF8
>+		 */

Same problem here for the multiple line comment

>+		if (g_at_result_iter_next_string(iter, &string)) {

Could the device omit the quote here? If so, this function couldn't handle it correctly either.

>+				*str = g_strdup(string);
>+				return TRUE;
>+		}
>+	}
>+	return FALSE;
> }
>
> static const char *best_charset(int supported)
>@@ -110,15 +144,15 @@ static void at_cpbr_notify(GAtResult *result, gpointer
>user_data)
> 		int index;
> 		const char *number;
> 		int type;
>-		const char *text;
>+		char *text;
> 		int hidden = -1;
>-		const char *group = NULL;
>+		char *group = NULL;
> 		const char *adnumber = NULL;
> 		int adtype = -1;
>-		const char *secondtext = NULL;
>-		const char *email = NULL;
>-		const char *sip_uri = NULL;
>-		const char *tel_uri = NULL;
>+		char *secondtext = NULL;
>+		char *email = NULL;
>+		char *sip_uri = NULL;
>+		char *tel_uri = NULL;
>
> 		if (!g_at_result_iter_next_number(&iter, &index))
> 			continue;
>@@ -129,70 +163,29 @@ static void at_cpbr_notify(GAtResult *result, gpointer
>user_data)
> 		if (!g_at_result_iter_next_number(&iter, &type))
> 			continue;
>
>-		if (!g_at_result_iter_next_string(&iter, &text))
>+		if (!parse_text(&iter, &text, current))
> 			continue;
>
> 		g_at_result_iter_next_number(&iter, &hidden);
>-		g_at_result_iter_next_string(&iter, &group);
>+		parse_text(&iter, &group, current);
> 		g_at_result_iter_next_string(&iter, &adnumber);
> 		g_at_result_iter_next_number(&iter, &adtype);
>-		g_at_result_iter_next_string(&iter, &secondtext);
>-		g_at_result_iter_next_string(&iter, &email);
>-		g_at_result_iter_next_string(&iter, &sip_uri);
>-		g_at_result_iter_next_string(&iter, &tel_uri);
>-
>-		/* charset_current is either CHARSET_UCS2 or CHARSET_UTF8 */
>-		if (current == CHARSET_UCS2) {
>-			char *text_utf8;
>-			char *group_utf8 = NULL;
>-			char *secondtext_utf8 = NULL;
>-			char *email_utf8 = NULL;
>-			char *sip_uri_utf8 = NULL;
>-			char *tel_uri_utf8 = NULL;
>-
>-			text_utf8 = ucs2_to_utf8(text);
>-
>-			if (text_utf8 == NULL)
>-				ofono_warn("Name field conversion to UTF8"
>-						" failed, this can indicate a"
>-						" problem with modem"
>-						" integration, as this field"
>-						" is required by 27.007."
>-						"  Contents of name reported"
>-						" by modem: %s", text);
>-
>-			if (group)
>-				group_utf8 = ucs2_to_utf8(group);
>-			if (secondtext)
>-				secondtext_utf8 = ucs2_to_utf8(secondtext);
>-			if (email)
>-				email_utf8 = ucs2_to_utf8(email);
>-			if (sip_uri)
>-				sip_uri_utf8 = ucs2_to_utf8(sip_uri);
>-			if (tel_uri)
>-				tel_uri_utf8 = ucs2_to_utf8(tel_uri);
>-
>-			ofono_phonebook_entry(pb, index, number, type,
>-				text_utf8, hidden, group_utf8, adnumber,
>-				adtype, secondtext_utf8, email_utf8,
>-				sip_uri_utf8, tel_uri_utf8);
>-
>-			g_free(text_utf8);
>-			g_free(group_utf8);
>-			g_free(secondtext_utf8);
>-			g_free(email_utf8);
>-			g_free(sip_uri_utf8);
>-			g_free(tel_uri_utf8);
>-		} else {
>-			/* In the case of IRA charset, assume these are Latin1
>-			 * characters, same as in UTF8
>-			 */
>-			ofono_phonebook_entry(pb, index, number, type,
>-				text, hidden, group, adnumber,
>-				adtype, secondtext, email,
>-				sip_uri, tel_uri);
>-
>-		}
>+		parse_text(&iter, &secondtext, current);
>+		parse_text(&iter, &email, current);
>+		parse_text(&iter, &sip_uri, current);
>+		parse_text(&iter, &tel_uri, current);
>+
>+		ofono_phonebook_entry(pb, index, number, type,
>+			text, hidden, group, adnumber,
>+			adtype, secondtext, email,
>+			sip_uri, tel_uri);
>+
>+		g_free(text);
>+		g_free(group);
>+		g_free(secondtext);
>+		g_free(email);
>+		g_free(sip_uri);
>+		g_free(tel_uri);
> 	}
> }
>
>--
>1.7.1
>
>_______________________________________________
>ofono mailing list
>ofono(a)ofono.org
>http://lists.ofono.org/listinfo/ofono



Regards,
-Yang

Re: [PATCH] atmodem/phonebook: parse text as hexstring

[Thadeu Lima de Souza Cascardo]

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

On Wed, Aug 11, 2010 at 03:53:36PM +0800, Gu, Yang wrote:
> Hi, 
> 

Hello, Yang.

Thanks for the comments. I've done this patch in a rush (have done a
worse one before to test and import some data from a cell phone). Your
feedback is important so we can improve it and I can have some doubts
answered.

> 
> >-----Original Message-----
> >From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)ofono.org] On Behalf Of
> >Thadeu Lima de Souza Cascardo
> >Sent: Monday, August 09, 2010 6:28 AM
> >To: ofono(a)ofono.org
> >Subject: [PATCH] atmodem/phonebook: parse text as hexstring
> >
> >Some modems omit the quotes when dumping strings in UCS2. Parsing
> >them as hexstring already does the hex decoding and accepts missing
> >quotes.
> >
> >Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> >---
> > drivers/atmodem/phonebook.c |  139 ++++++++++++++++++++----------------------
> > 1 files changed, 66 insertions(+), 73 deletions(-)
> >
> >diff --git a/drivers/atmodem/phonebook.c b/drivers/atmodem/phonebook.c
> >index 5e7a52b..59fcbbc 100644
> >--- a/drivers/atmodem/phonebook.c
> >+++ b/drivers/atmodem/phonebook.c
> >@@ -59,16 +59,50 @@ struct pb_data {
> > 	GAtChat *chat;
> > };
> >
> >-static char *ucs2_to_utf8(const char *str)
> >+static void warn_bad_text(const char *text)
> > {
> >-	long len;
> >-	unsigned char *ucs2;
> >+	ofono_warn("Name field conversion to UTF8"
> >+		" failed, this can indicate a"
> >+		" problem with modem"
> >+		" integration, as this field"
> >+		" is required by 27.007."
> >+		"  Contents of name reported"
> >+		" by modem: %s", text);
> >+}
> 
> Why this warning is extracted as a function here?
> 

In one of my versions of the patch, it was too much inlined and the
function was already too long. Since it's static, it will probably be
inlined. Should I just put it back in place of its only caller?

> >+
> >+static gboolean parse_text(GAtResultIter *iter, char **str, int encoding)
> >+{
> >+	const char *string;
> >+	const guint8 *hex;
> >+	int len;
> > 	char *utf8;
> >-	ucs2 = decode_hex(str, -1, &len, 0);
> >-	utf8 = g_convert((char *)ucs2, len, "UTF-8//TRANSLIT", "UCS-2BE",
> >-					NULL, NULL, NULL);
> >-	g_free(ucs2);
> >-	return utf8;
> >+	/* charset_current is either CHARSET_UCS2 or CHARSET_UTF8 */
> 
> Please help to add CHARSET_IRA here. I know the original code is not correct.
> 

OK. Any pointers to any documentation about it? UCS2 support worked very
well for me. Some names had characters in the Latin1 range and it was no
problem.

> >+	if (encoding == CHARSET_UCS2) {
> >+		/* Some devices ommit the quotes, so use hexstring,
> >+		 * which also decode to hex */
> 
> Misspell the "omit" here. We have guideline to write multiple line comments, please refer doc/coding_style.txt and section M2.
> 

Thanks. I will take a look and fix those.

> >+		if (g_at_result_iter_next_hexstring(iter, &hex, &len))
> >+			utf8 = g_convert((const gchar *)hex, len,
> >+						"UTF-8//TRANSLIT", "UCS-2BE",
> >+						NULL, NULL, NULL);
> >+		else
> >+			return FALSE;
> 
> Please insert a blank line here.
> 
> >+		if (utf8) {
> >+			*str = utf8;
> >+			return TRUE;
> >+		} else {
> >+			warn_bad_text(string);
> 
> We only report the warning when the field "name" (i.e., text) couldn't be converted correctly, for it's a mandatory field. Otherwise, we don't issue the warning.
> 

I will add a gboolean mandatory parameter to the function. I've thought
about that, but now I have a pretty good line (mandatory), that's pretty
clear.

> >+			return FALSE;
> >+		}
> >+	} else {
> >+		/* In the case of IRA charset, assume these are Latin1
> >+		 * characters, same as in UTF8
> >+		 */
> 
> Same problem here for the multiple line comment
> 
> >+		if (g_at_result_iter_next_string(iter, &string)) {
> 
> Could the device omit the quote here? If so, this function couldn't handle it correctly either.
> 

Not the device I have. This does not add a regression. If we find any
device that will omit the quote in this case, we may add the fix later.
For now, I am more confortable requiring the quote here and fixing it
for the cases we know some devices will omit it.

In any case, the UCS2 case was already expecting an hexstring, since it
was doing the conversion later. And the hexstring code allowed the
quotes to be omited. This is not the case here, where a string is
expected. I don't know about the IRA case.

> >+				*str = g_strdup(string);
> >+				return TRUE;
> >+		}
> >+	}
> >+	return FALSE;
> > }
> >
> > static const char *best_charset(int supported)
> >@@ -110,15 +144,15 @@ static void at_cpbr_notify(GAtResult *result, gpointer
> >user_data)
> > 		int index;
> > 		const char *number;
> > 		int type;
> >-		const char *text;
> >+		char *text;
> > 		int hidden = -1;
> >-		const char *group = NULL;
> >+		char *group = NULL;
> > 		const char *adnumber = NULL;
> > 		int adtype = -1;
> >-		const char *secondtext = NULL;
> >-		const char *email = NULL;
> >-		const char *sip_uri = NULL;
> >-		const char *tel_uri = NULL;
> >+		char *secondtext = NULL;
> >+		char *email = NULL;
> >+		char *sip_uri = NULL;
> >+		char *tel_uri = NULL;
> >
> > 		if (!g_at_result_iter_next_number(&iter, &index))
> > 			continue;
> >@@ -129,70 +163,29 @@ static void at_cpbr_notify(GAtResult *result, gpointer
> >user_data)
> > 		if (!g_at_result_iter_next_number(&iter, &type))
> > 			continue;
> >
> >-		if (!g_at_result_iter_next_string(&iter, &text))
> >+		if (!parse_text(&iter, &text, current))
> > 			continue;
> >
> > 		g_at_result_iter_next_number(&iter, &hidden);
> >-		g_at_result_iter_next_string(&iter, &group);
> >+		parse_text(&iter, &group, current);
> > 		g_at_result_iter_next_string(&iter, &adnumber);
> > 		g_at_result_iter_next_number(&iter, &adtype);
> >-		g_at_result_iter_next_string(&iter, &secondtext);
> >-		g_at_result_iter_next_string(&iter, &email);
> >-		g_at_result_iter_next_string(&iter, &sip_uri);
> >-		g_at_result_iter_next_string(&iter, &tel_uri);
> >-
> >-		/* charset_current is either CHARSET_UCS2 or CHARSET_UTF8 */
> >-		if (current == CHARSET_UCS2) {
> >-			char *text_utf8;
> >-			char *group_utf8 = NULL;
> >-			char *secondtext_utf8 = NULL;
> >-			char *email_utf8 = NULL;
> >-			char *sip_uri_utf8 = NULL;
> >-			char *tel_uri_utf8 = NULL;
> >-
> >-			text_utf8 = ucs2_to_utf8(text);
> >-
> >-			if (text_utf8 == NULL)
> >-				ofono_warn("Name field conversion to UTF8"
> >-						" failed, this can indicate a"
> >-						" problem with modem"
> >-						" integration, as this field"
> >-						" is required by 27.007."
> >-						"  Contents of name reported"
> >-						" by modem: %s", text);
> >-
> >-			if (group)
> >-				group_utf8 = ucs2_to_utf8(group);
> >-			if (secondtext)
> >-				secondtext_utf8 = ucs2_to_utf8(secondtext);
> >-			if (email)
> >-				email_utf8 = ucs2_to_utf8(email);
> >-			if (sip_uri)
> >-				sip_uri_utf8 = ucs2_to_utf8(sip_uri);
> >-			if (tel_uri)
> >-				tel_uri_utf8 = ucs2_to_utf8(tel_uri);
> >-
> >-			ofono_phonebook_entry(pb, index, number, type,
> >-				text_utf8, hidden, group_utf8, adnumber,
> >-				adtype, secondtext_utf8, email_utf8,
> >-				sip_uri_utf8, tel_uri_utf8);
> >-
> >-			g_free(text_utf8);
> >-			g_free(group_utf8);
> >-			g_free(secondtext_utf8);
> >-			g_free(email_utf8);
> >-			g_free(sip_uri_utf8);
> >-			g_free(tel_uri_utf8);
> >-		} else {
> >-			/* In the case of IRA charset, assume these are Latin1
> >-			 * characters, same as in UTF8
> >-			 */
> >-			ofono_phonebook_entry(pb, index, number, type,
> >-				text, hidden, group, adnumber,
> >-				adtype, secondtext, email,
> >-				sip_uri, tel_uri);
> >-
> >-		}
> >+		parse_text(&iter, &secondtext, current);
> >+		parse_text(&iter, &email, current);
> >+		parse_text(&iter, &sip_uri, current);
> >+		parse_text(&iter, &tel_uri, current);
> >+
> >+		ofono_phonebook_entry(pb, index, number, type,
> >+			text, hidden, group, adnumber,
> >+			adtype, secondtext, email,
> >+			sip_uri, tel_uri);
> >+
> >+		g_free(text);
> >+		g_free(group);
> >+		g_free(secondtext);
> >+		g_free(email);
> >+		g_free(sip_uri);
> >+		g_free(tel_uri);
> > 	}
> > }
> >
> >--
> >1.7.1
> 
> Regards,
> -Yang

Regards,
Cascardo.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] atmodem/phonebook: parse text as hexstring

[Denis Kenzior]

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

Hi Cascardo,

>>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>

We don't use Signed-Off here, so please don't use it when sending
patches for oFono.

>>> ---
>>> drivers/atmodem/phonebook.c |  139 ++++++++++++++++++++----------------------
>>> 1 files changed, 66 insertions(+), 73 deletions(-)
>>>
>>> diff --git a/drivers/atmodem/phonebook.c b/drivers/atmodem/phonebook.c
>>> index 5e7a52b..59fcbbc 100644
>>> --- a/drivers/atmodem/phonebook.c
>>> +++ b/drivers/atmodem/phonebook.c
>>> @@ -59,16 +59,50 @@ struct pb_data {
>>> 	GAtChat *chat;
>>> };
>>>
>>> -static char *ucs2_to_utf8(const char *str)
>>> +static void warn_bad_text(const char *text)
>>> {
>>> -	long len;
>>> -	unsigned char *ucs2;
>>> +	ofono_warn("Name field conversion to UTF8"
>>> +		" failed, this can indicate a"
>>> +		" problem with modem"
>>> +		" integration, as this field"
>>> +		" is required by 27.007."
>>> +		"  Contents of name reported"
>>> +		" by modem: %s", text);
>>> +}
>>
>> Why this warning is extracted as a function here?
>>
> 
> In one of my versions of the patch, it was too much inlined and the
> function was already too long. Since it's static, it will probably be
> inlined. Should I just put it back in place of its only caller?
> 

This is actually fine, but please re-flow the text to take advantage of
the available space.  There's no reason to stick with 50 character
maximum now.

>>> +		if (utf8) {
>>> +			*str = utf8;
>>> +			return TRUE;
>>> +		} else {
>>> +			warn_bad_text(string);
>>
>> We only report the warning when the field "name" (i.e., text) couldn't be converted correctly, for it's a mandatory field. Otherwise, we don't issue the warning.
>>
> 
> I will add a gboolean mandatory parameter to the function. I've thought
> about that, but now I have a pretty good line (mandatory), that's pretty
> clear.
> 

Please do this.

>>> +		if (g_at_result_iter_next_string(iter, &string)) {
>>
>> Could the device omit the quote here? If so, this function couldn't handle it correctly either.
>>
> 
> Not the device I have. This does not add a regression. If we find any
> device that will omit the quote in this case, we may add the fix later.
> For now, I am more confortable requiring the quote here and fixing it
> for the cases we know some devices will omit it.
> 
> In any case, the UCS2 case was already expecting an hexstring, since it
> was doing the conversion later. And the hexstring code allowed the
> quotes to be omited. This is not the case here, where a string is
> expected. I don't know about the IRA case.
> 

I agree, the standard says that all strings should be in quotes.  If
some modem does not follow this standard then we will need to quirk it
specifically.  For now I think this is fine.

Please fix up the other style issues commented on by Yang and resubmit
this patch.

Regards,
-Denis

Re: [PATCH] atmodem/phonebook: parse text as hexstring

[Denis Kenzior]

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

Hi Thadeu,

On 08/13/2010 10:04 PM, Thadeu Lima de Souza Cascardo wrote:
> Some modems omit the quotes when dumping strings in UCS2. Parsing
> them as hexstring already does the hex decoding and accepts missing
> quotes.
> ---
>  drivers/atmodem/phonebook.c |  142 +++++++++++++++++++++----------------------
>  1 files changed, 69 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/atmodem/phonebook.c b/drivers/atmodem/phonebook.c
> index 89d090b..9e376d3 100644
> --- a/drivers/atmodem/phonebook.c
> +++ b/drivers/atmodem/phonebook.c
> @@ -59,16 +59,53 @@ struct pb_data {
>  	GAtChat *chat;
>  };
>  
> -static char *ucs2_to_utf8(const char *str)
> +static void warn_bad_text(const char *text)
>  {
> -	long len;
> -	unsigned char *ucs2;
> +	ofono_warn("Name field conversion to UTF8 failed, this can indicate a"
> +		" problem with modem integration, as this field"
> +		" is required by 27.007.  Contents of name reported"
> +		" by modem: %s", text);
> +}
> +
> +static gboolean parse_text(GAtResultIter *iter, char **str, int encoding,
> +			   gboolean mandatory)

Please don't mix tabs and spaces for indentation.  We only use tabs for
oFono.

> +{
> +	const char *string;
> +	const guint8 *hex;
> +	int len;
>  	char *utf8;
> -	ucs2 = decode_hex(str, -1, &len, 0);
> -	utf8 = g_convert((char *)ucs2, len, "UTF-8//TRANSLIT", "UCS-2BE",
> -					NULL, NULL, NULL);
> -	g_free(ucs2);
> -	return utf8;
> +	/* charset_current is CHARSET_UCS2, CHARSET_IRA or CHARSET_UTF8 */
> +	if (encoding == CHARSET_UCS2) {
> +		/*
> +		 * Some devices omit the quotes, so use hexstring,
> +		 * which also decode to hex
> +		 */
> +		if (g_at_result_iter_next_hexstring(iter, &hex, &len))
> +			utf8 = g_convert((const gchar*) hex, len,
> +						"UTF-8//TRANSLIT", "UCS-2BE",
> +						NULL, NULL, NULL);
> +		else
> +			return FALSE;

I'd really prefer something like:
if (g_at_result_iter_next_hexstring() == FALSE)
	return FALSE;

utf = g_convert();

> +
> +		if (utf8) {
> +			*str = utf8;
> +			return TRUE;
> +		} else {
> +			if (mandatory)
> +				warn_bad_text(string);

string is used uninitialized here...

> +			return FALSE;
> +		}

Same this for the above if-else block:

if (utf8) {
	*str = utf8;
	return TRUE;
}

if (mandatory)
	....

return FALSE

> +	} else {
> +		/*
> +		 * In the case of IRA charset, assume these are Latin1
> +		 * characters, same as in UTF8
> +		 */
> +		if (g_at_result_iter_next_string(iter, &string)) {
> +				*str = g_strdup(string);
> +				return TRUE;

This one is indented 1 too many...

> +		}

This doesn't need to be in an else clause since you return in all cases
above...

> +	}

You need an empty line here...

> +	return FALSE;
>  }
>  
>  static const char *best_charset(int supported)
> @@ -110,15 +147,15 @@ static void at_cpbr_notify(GAtResult *result, gpointer user_data)
>  		int index;
>  		const char *number;
>  		int type;
> -		const char *text;
> +		char *text;
>  		int hidden = -1;
> -		const char *group = NULL;
> +		char *group = NULL;
>  		const char *adnumber = NULL;
>  		int adtype = -1;
> -		const char *secondtext = NULL;
> -		const char *email = NULL;
> -		const char *sip_uri = NULL;
> -		const char *tel_uri = NULL;
> +		char *secondtext = NULL;
> +		char *email = NULL;
> +		char *sip_uri = NULL;
> +		char *tel_uri = NULL;
>  
>  		if (!g_at_result_iter_next_number(&iter, &index))
>  			continue;
> @@ -129,70 +166,29 @@ static void at_cpbr_notify(GAtResult *result, gpointer user_data)
>  		if (!g_at_result_iter_next_number(&iter, &type))
>  			continue;
>  
> -		if (!g_at_result_iter_next_string(&iter, &text))
> +		if (!parse_text(&iter, &text, current, TRUE))
>  			continue;

Now that I thought it about some more, lets get rid of the mandatory
argument to parse text.  Instead warn_bad if parse_text fails here.

e.g.:

if (parse_text() == FALSE) {
	warn_bad();
	continue;
}

>  
>  		g_at_result_iter_next_number(&iter, &hidden);
> -		g_at_result_iter_next_string(&iter, &group);
> +		parse_text(&iter, &group, current, FALSE);
>  		g_at_result_iter_next_string(&iter, &adnumber);
>  		g_at_result_iter_next_number(&iter, &adtype);
> -		g_at_result_iter_next_string(&iter, &secondtext);
> -		g_at_result_iter_next_string(&iter, &email);
> -		g_at_result_iter_next_string(&iter, &sip_uri);
> -		g_at_result_iter_next_string(&iter, &tel_uri);
> -
> -		/* charset_current is either CHARSET_UCS2 or CHARSET_UTF8 */
> -		if (current == CHARSET_UCS2) {
> -			char *text_utf8;
> -			char *group_utf8 = NULL;
> -			char *secondtext_utf8 = NULL;
> -			char *email_utf8 = NULL;
> -			char *sip_uri_utf8 = NULL;
> -			char *tel_uri_utf8 = NULL;
> -
> -			text_utf8 = ucs2_to_utf8(text);
> -
> -			if (text_utf8 == NULL)
> -				ofono_warn("Name field conversion to UTF8"
> -						" failed, this can indicate a"
> -						" problem with modem"
> -						" integration, as this field"
> -						" is required by 27.007."
> -						"  Contents of name reported"
> -						" by modem: %s", text);
> -
> -			if (group)
> -				group_utf8 = ucs2_to_utf8(group);
> -			if (secondtext)
> -				secondtext_utf8 = ucs2_to_utf8(secondtext);
> -			if (email)
> -				email_utf8 = ucs2_to_utf8(email);
> -			if (sip_uri)
> -				sip_uri_utf8 = ucs2_to_utf8(sip_uri);
> -			if (tel_uri)
> -				tel_uri_utf8 = ucs2_to_utf8(tel_uri);
> -
> -			ofono_phonebook_entry(pb, index, number, type,
> -				text_utf8, hidden, group_utf8, adnumber,
> -				adtype, secondtext_utf8, email_utf8,
> -				sip_uri_utf8, tel_uri_utf8);
> -
> -			g_free(text_utf8);
> -			g_free(group_utf8);
> -			g_free(secondtext_utf8);
> -			g_free(email_utf8);
> -			g_free(sip_uri_utf8);
> -			g_free(tel_uri_utf8);
> -		} else {
> -			/* In the case of IRA charset, assume these are Latin1
> -			 * characters, same as in UTF8
> -			 */
> -			ofono_phonebook_entry(pb, index, number, type,
> -				text, hidden, group, adnumber,
> -				adtype, secondtext, email,
> -				sip_uri, tel_uri);
> -
> -		}
> +		parse_text(&iter, &secondtext, current, FALSE);
> +		parse_text(&iter, &email, current, FALSE);
> +		parse_text(&iter, &sip_uri, current, FALSE);
> +		parse_text(&iter, &tel_uri, current, FALSE);
> +
> +		ofono_phonebook_entry(pb, index, number, type,
> +			text, hidden, group, adnumber,
> +			adtype, secondtext, email,
> +			sip_uri, tel_uri);
> +
> +		g_free(text);
> +		g_free(group);
> +		g_free(secondtext);
> +		g_free(email);
> +		g_free(sip_uri);
> +		g_free(tel_uri);
>  	}
>  }
>  

Regards,
-Denis

[PATCH] atmodem/phonebook: parse text as hexstring

[Thadeu Lima de Souza Cascardo]

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

Some modems omit the quotes when dumping strings in UCS2. Parsing
them as hexstring already does the hex decoding and accepts missing
quotes.
---
 drivers/atmodem/phonebook.c |  142 +++++++++++++++++++++----------------------
 1 files changed, 69 insertions(+), 73 deletions(-)

diff --git a/drivers/atmodem/phonebook.c b/drivers/atmodem/phonebook.c
index 89d090b..9e376d3 100644
--- a/drivers/atmodem/phonebook.c
+++ b/drivers/atmodem/phonebook.c
@@ -59,16 +59,53 @@ struct pb_data {
 	GAtChat *chat;
 };
 
-static char *ucs2_to_utf8(const char *str)
+static void warn_bad_text(const char *text)
 {
-	long len;
-	unsigned char *ucs2;
+	ofono_warn("Name field conversion to UTF8 failed, this can indicate a"
+		" problem with modem integration, as this field"
+		" is required by 27.007.  Contents of name reported"
+		" by modem: %s", text);
+}
+
+static gboolean parse_text(GAtResultIter *iter, char **str, int encoding,
+			   gboolean mandatory)
+{
+	const char *string;
+	const guint8 *hex;
+	int len;
 	char *utf8;
-	ucs2 = decode_hex(str, -1, &len, 0);
-	utf8 = g_convert((char *)ucs2, len, "UTF-8//TRANSLIT", "UCS-2BE",
-					NULL, NULL, NULL);
-	g_free(ucs2);
-	return utf8;
+	/* charset_current is CHARSET_UCS2, CHARSET_IRA or CHARSET_UTF8 */
+	if (encoding == CHARSET_UCS2) {
+		/*
+		 * Some devices omit the quotes, so use hexstring,
+		 * which also decode to hex
+		 */
+		if (g_at_result_iter_next_hexstring(iter, &hex, &len))
+			utf8 = g_convert((const gchar*) hex, len,
+						"UTF-8//TRANSLIT", "UCS-2BE",
+						NULL, NULL, NULL);
+		else
+			return FALSE;
+
+		if (utf8) {
+			*str = utf8;
+			return TRUE;
+		} else {
+			if (mandatory)
+				warn_bad_text(string);
+			return FALSE;
+		}
+	} else {
+		/*
+		 * In the case of IRA charset, assume these are Latin1
+		 * characters, same as in UTF8
+		 */
+		if (g_at_result_iter_next_string(iter, &string)) {
+				*str = g_strdup(string);
+				return TRUE;
+		}
+	}
+	return FALSE;
 }
 
 static const char *best_charset(int supported)
@@ -110,15 +147,15 @@ static void at_cpbr_notify(GAtResult *result, gpointer user_data)
 		int index;
 		const char *number;
 		int type;
-		const char *text;
+		char *text;
 		int hidden = -1;
-		const char *group = NULL;
+		char *group = NULL;
 		const char *adnumber = NULL;
 		int adtype = -1;
-		const char *secondtext = NULL;
-		const char *email = NULL;
-		const char *sip_uri = NULL;
-		const char *tel_uri = NULL;
+		char *secondtext = NULL;
+		char *email = NULL;
+		char *sip_uri = NULL;
+		char *tel_uri = NULL;
 
 		if (!g_at_result_iter_next_number(&iter, &index))
 			continue;
@@ -129,70 +166,29 @@ static void at_cpbr_notify(GAtResult *result, gpointer user_data)
 		if (!g_at_result_iter_next_number(&iter, &type))
 			continue;
 
-		if (!g_at_result_iter_next_string(&iter, &text))
+		if (!parse_text(&iter, &text, current, TRUE))
 			continue;
 
 		g_at_result_iter_next_number(&iter, &hidden);
-		g_at_result_iter_next_string(&iter, &group);
+		parse_text(&iter, &group, current, FALSE);
 		g_at_result_iter_next_string(&iter, &adnumber);
 		g_at_result_iter_next_number(&iter, &adtype);
-		g_at_result_iter_next_string(&iter, &secondtext);
-		g_at_result_iter_next_string(&iter, &email);
-		g_at_result_iter_next_string(&iter, &sip_uri);
-		g_at_result_iter_next_string(&iter, &tel_uri);
-
-		/* charset_current is either CHARSET_UCS2 or CHARSET_UTF8 */
-		if (current == CHARSET_UCS2) {
-			char *text_utf8;
-			char *group_utf8 = NULL;
-			char *secondtext_utf8 = NULL;
-			char *email_utf8 = NULL;
-			char *sip_uri_utf8 = NULL;
-			char *tel_uri_utf8 = NULL;
-
-			text_utf8 = ucs2_to_utf8(text);
-
-			if (text_utf8 == NULL)
-				ofono_warn("Name field conversion to UTF8"
-						" failed, this can indicate a"
-						" problem with modem"
-						" integration, as this field"
-						" is required by 27.007."
-						"  Contents of name reported"
-						" by modem: %s", text);
-
-			if (group)
-				group_utf8 = ucs2_to_utf8(group);
-			if (secondtext)
-				secondtext_utf8 = ucs2_to_utf8(secondtext);
-			if (email)
-				email_utf8 = ucs2_to_utf8(email);
-			if (sip_uri)
-				sip_uri_utf8 = ucs2_to_utf8(sip_uri);
-			if (tel_uri)
-				tel_uri_utf8 = ucs2_to_utf8(tel_uri);
-
-			ofono_phonebook_entry(pb, index, number, type,
-				text_utf8, hidden, group_utf8, adnumber,
-				adtype, secondtext_utf8, email_utf8,
-				sip_uri_utf8, tel_uri_utf8);
-
-			g_free(text_utf8);
-			g_free(group_utf8);
-			g_free(secondtext_utf8);
-			g_free(email_utf8);
-			g_free(sip_uri_utf8);
-			g_free(tel_uri_utf8);
-		} else {
-			/* In the case of IRA charset, assume these are Latin1
-			 * characters, same as in UTF8
-			 */
-			ofono_phonebook_entry(pb, index, number, type,
-				text, hidden, group, adnumber,
-				adtype, secondtext, email,
-				sip_uri, tel_uri);
-
-		}
+		parse_text(&iter, &secondtext, current, FALSE);
+		parse_text(&iter, &email, current, FALSE);
+		parse_text(&iter, &sip_uri, current, FALSE);
+		parse_text(&iter, &tel_uri, current, FALSE);
+
+		ofono_phonebook_entry(pb, index, number, type,
+			text, hidden, group, adnumber,
+			adtype, secondtext, email,
+			sip_uri, tel_uri);
+
+		g_free(text);
+		g_free(group);
+		g_free(secondtext);
+		g_free(email);
+		g_free(sip_uri);
+		g_free(tel_uri);
 	}
 }
 
-- 
1.7.1