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

[Denis Kenzior <denkenz@gmail.com>]

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