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: [email protected] [mailto:[email protected]] On Behalf Of > >Thadeu Lima de Souza Cascardo > >Sent: Monday, August 09, 2010 6:28 AM > >To: [email protected] > >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 <[email protected]> > >--- > > 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.
signature.asc
Description: Digital signature
_______________________________________________ ofono mailing list [email protected] http://lists.ofono.org/listinfo/ofono
