Re: [PATCH] util: Fix GSM to UTF8 conversion mechanism

Re: [PATCH] util: Fix GSM to UTF8 conversion mechanism

[Denis Kenzior]

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

Hi Guillaume,

On 07/16/2012 09:34 AM, Guillaume Zajac wrote:
> ---
>   src/util.c |   17 ++++++++++++++---
>   1 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/src/util.c b/src/util.c
> index aecc790..41ef3d4 100644
> --- a/src/util.c
> +++ b/src/util.c
> @@ -628,8 +628,16 @@ char *convert_gsm_to_utf8_with_lang(const unsigned char *text, long len,
>
>   			c = gsm_single_shift_lookup(&t, text[i]);
>
> +			/*
> +			 * According to the 3GPP specifications 23.038
> +			 * section 6.2.1.1:
> +			 * In the case there is no character in the extension
> +			 * table, the character of the main default alphabet
> +			 * table or the character from the National Language
> +			 * Locking Shift Table should be displayed.
> +			 */

What version of 23.038 are you using? I cannot find such wording in R9 
or R10 specs.

>   			if (c == GUND)
> -				goto error;
> +				c = gsm_locking_shift_lookup(&t, text[i]);
>   		} else {
>   			c = gsm_locking_shift_lookup(&t, text[i]);
>   		}
> @@ -647,9 +655,12 @@ char *convert_gsm_to_utf8_with_lang(const unsigned char *text, long len,
>   	while (out<  res + res_length) {
>   		unsigned short c;
>
> -		if (text[i] == 0x1b)
> +		if (text[i] == 0x1b) {
>   			c = gsm_single_shift_lookup(&t, text[++i]);
> -		else
> +			/* See 3GPP 23.038 section 6.2.1.1 */
> +			if (c == GUND)
> +				c = gsm_locking_shift_lookup(&t, text[i]);
> +		} else
>   			c = gsm_locking_shift_lookup(&t, text[i]);
>
>   		out += g_unichar_to_utf8(c, out);

Regards,
-Denis

[PATCH] util: Fix GSM to UTF8 conversion mechanism

[Guillaume Zajac]

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

---
 src/util.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/util.c b/src/util.c
index aecc790..41ef3d4 100644
--- a/src/util.c
+++ b/src/util.c
@@ -628,8 +628,16 @@ char *convert_gsm_to_utf8_with_lang(const unsigned char *text, long len,
 
 			c = gsm_single_shift_lookup(&t, text[i]);
 
+			/*
+			 * According to the 3GPP specifications 23.038
+			 * section 6.2.1.1:
+			 * In the case there is no character in the extension
+			 * table, the character of the main default alphabet
+			 * table or the character from the National Language
+			 * Locking Shift Table should be displayed.
+			 */
 			if (c == GUND)
-				goto error;
+				c = gsm_locking_shift_lookup(&t, text[i]);
 		} else {
 			c = gsm_locking_shift_lookup(&t, text[i]);
 		}
@@ -647,9 +655,12 @@ char *convert_gsm_to_utf8_with_lang(const unsigned char *text, long len,
 	while (out < res + res_length) {
 		unsigned short c;
 
-		if (text[i] == 0x1b)
+		if (text[i] == 0x1b) {
 			c = gsm_single_shift_lookup(&t, text[++i]);
-		else
+			/* See 3GPP 23.038 section 6.2.1.1 */
+			if (c == GUND)
+				c = gsm_locking_shift_lookup(&t, text[i]);
+		} else
 			c = gsm_locking_shift_lookup(&t, text[i]);
 
 		out += g_unichar_to_utf8(c, out);
-- 
1.7.5.4

Re: [PATCH] util: Fix GSM to UTF8 conversion mechanism

[Denis Kenzior]

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

Hi Guillaume,

>>> + /*
>>> + * According to the 3GPP specifications 23.038
>>> + * section 6.2.1.1:
>>> + * In the case there is no character in the extension
>>> + * table, the character of the main default alphabet
>>> + * table or the character from the National Language
>>> + * Locking Shift Table should be displayed.
>>> + */
>>
>> What version of 23.038 are you using? I cannot find such wording in R9
>> or R10 specs.
>
> It is not the exact wording, I have just rephrased the comment.
> The exact wording it just below the GSM 7 bits default alphabet
> extension table in section 6.2.1.1
> Generally, do you prefer having an exact extract of the 3GPP spec or
> just an explanation of what is happening?
>

The preference is to quote the spec verbatim and enclose the quote in 
"".  You can quote fragments of a sentence, etc.

You can paraphrase, but I prefer that we only do that to distill 
something simple, e.g. maximum size of a structure/member.

Regards,
-Denis

Re: [PATCH] util: Fix GSM to UTF8 conversion mechanism

[Denis Kenzior]

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

Hi Guillaume,

On 07/16/2012 09:34 AM, Guillaume Zajac wrote:
> ---
>   src/util.c |   17 ++++++++++++++---
>   1 files changed, 14 insertions(+), 3 deletions(-)
>

After finding the aforementioned verbiage in the specs I went ahead and 
pushed this patch.  I did update the comment to cite the specification 
exactly in a follow on commit.

Thanks,
-Denis

Re: [PATCH] util: Fix GSM to UTF8 conversion mechanism

[Guillaume Zajac]

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

Hi Denis,

On 16/07/2012 02:49, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 07/16/2012 09:34 AM, Guillaume Zajac wrote:
>> ---
>>   src/util.c |   17 ++++++++++++++---
>>   1 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/util.c b/src/util.c
>> index aecc790..41ef3d4 100644
>> --- a/src/util.c
>> +++ b/src/util.c
>> @@ -628,8 +628,16 @@ char *convert_gsm_to_utf8_with_lang(const 
>> unsigned char *text, long len,
>>
>>               c = gsm_single_shift_lookup(&t, text[i]);
>>
>> +            /*
>> +             * According to the 3GPP specifications 23.038
>> +             * section 6.2.1.1:
>> +             * In the case there is no character in the extension
>> +             * table, the character of the main default alphabet
>> +             * table or the character from the National Language
>> +             * Locking Shift Table should be displayed.
>> +             */
>
> What version of 23.038 are you using? I cannot find such wording in R9 
> or R10 specs.

It is not the exact wording, I have just rephrased the comment.
The exact wording it just below the GSM 7 bits default alphabet 
extension table in section 6.2.1.1
Generally, do you prefer having an exact extract of the 3GPP spec or 
just an explanation of what is happening?

>
>>               if (c == GUND)
>> -                goto error;
>> +                c = gsm_locking_shift_lookup(&t, text[i]);
>>           } else {
>>               c = gsm_locking_shift_lookup(&t, text[i]);
>>           }
>> @@ -647,9 +655,12 @@ char *convert_gsm_to_utf8_with_lang(const 
>> unsigned char *text, long len,
>>       while (out<  res + res_length) {
>>           unsigned short c;
>>
>> -        if (text[i] == 0x1b)
>> +        if (text[i] == 0x1b) {
>>               c = gsm_single_shift_lookup(&t, text[++i]);
>> -        else
>> +            /* See 3GPP 23.038 section 6.2.1.1 */
>> +            if (c == GUND)
>> +                c = gsm_locking_shift_lookup(&t, text[i]);
>> +        } else
>>               c = gsm_locking_shift_lookup(&t, text[i]);
>>
>>           out += g_unichar_to_utf8(c, out);
>
> Regards,
> -Denis
>

Kind regards,
Guillaume

Re: [PATCH] util: Fix GSM to UTF8 conversion mechanism

[Denis Kenzior]

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

Hi Guillaume,

>> and can someone please run the unit tests if we are touching this:
>>
>> /testutil/Invalid Conversions: **
>> ERROR:unit/test-util.c:362:test_invalid: assertion failed: (res == NULL)
>> Aborted (core dumped)
>
> The purpose of this test was to check if we were failing to convert an
> invalid GSM extended character.
> As this was not the 3GPP recommended behavior, it sounds senseless to
> have it in test_invalid() now.
>
> Denis, should we modify it in checking the character of the main default
> alphabet is returned and add it in test_valid() section?

Yes, please update the tests.

Regards,
-Denis

Re: [PATCH] util: Fix GSM to UTF8 conversion mechanism

[Guillaume Zajac]

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

Hi Denis,

On 17/07/2012 04:09, Denis Kenzior wrote:
> Hi Guillaume,
>
>>>> + /*
>>>> + * According to the 3GPP specifications 23.038
>>>> + * section 6.2.1.1:
>>>> + * In the case there is no character in the extension
>>>> + * table, the character of the main default alphabet
>>>> + * table or the character from the National Language
>>>> + * Locking Shift Table should be displayed.
>>>> + */
>>>
>>> What version of 23.038 are you using? I cannot find such wording in R9
>>> or R10 specs.
>>
>> It is not the exact wording, I have just rephrased the comment.
>> The exact wording it just below the GSM 7 bits default alphabet
>> extension table in section 6.2.1.1
>> Generally, do you prefer having an exact extract of the 3GPP spec or
>> just an explanation of what is happening?
>>
>
> The preference is to quote the spec verbatim and enclose the quote in 
> "".  You can quote fragments of a sentence, etc.
>
> You can paraphrase, but I prefer that we only do that to distill 
> something simple, e.g. maximum size of a structure/member.
>

Ok, I keep his in mind for the next patches.


Kind regards,
Guillaume

Re: [PATCH] util: Fix GSM to UTF8 conversion mechanism

[Marcel Holtmann]

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

Hi Guillaume,

> >>>> + /*
> >>>> + * According to the 3GPP specifications 23.038
> >>>> + * section 6.2.1.1:
> >>>> + * In the case there is no character in the extension
> >>>> + * table, the character of the main default alphabet
> >>>> + * table or the character from the National Language
> >>>> + * Locking Shift Table should be displayed.
> >>>> + */
> >>>
> >>> What version of 23.038 are you using? I cannot find such wording in R9
> >>> or R10 specs.
> >>
> >> It is not the exact wording, I have just rephrased the comment.
> >> The exact wording it just below the GSM 7 bits default alphabet
> >> extension table in section 6.2.1.1
> >> Generally, do you prefer having an exact extract of the 3GPP spec or
> >> just an explanation of what is happening?
> >>
> >
> > The preference is to quote the spec verbatim and enclose the quote in 
> > "".  You can quote fragments of a sentence, etc.
> >
> > You can paraphrase, but I prefer that we only do that to distill 
> > something simple, e.g. maximum size of a structure/member.
> >
> 
> Ok, I keep his in mind for the next patches.

and can someone please run the unit tests if we are touching this:

/testutil/Invalid Conversions: **
ERROR:unit/test-util.c:362:test_invalid: assertion failed: (res == NULL)
Aborted (core dumped)

Regards

Marcel

Re: [PATCH] util: Fix GSM to UTF8 conversion mechanism

[Guillaume Zajac]

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

Hi,

On 18/07/2012 14:31, Marcel Holtmann wrote:
> Hi Guillaume,
>
>>>>>> + /*
>>>>>> + * According to the 3GPP specifications 23.038
>>>>>> + * section 6.2.1.1:
>>>>>> + * In the case there is no character in the extension
>>>>>> + * table, the character of the main default alphabet
>>>>>> + * table or the character from the National Language
>>>>>> + * Locking Shift Table should be displayed.
>>>>>> + */
>>>>> What version of 23.038 are you using? I cannot find such wording in R9
>>>>> or R10 specs.
>>>> It is not the exact wording, I have just rephrased the comment.
>>>> The exact wording it just below the GSM 7 bits default alphabet
>>>> extension table in section 6.2.1.1
>>>> Generally, do you prefer having an exact extract of the 3GPP spec or
>>>> just an explanation of what is happening?
>>>>
>>> The preference is to quote the spec verbatim and enclose the quote in
>>> "".  You can quote fragments of a sentence, etc.
>>>
>>> You can paraphrase, but I prefer that we only do that to distill
>>> something simple, e.g. maximum size of a structure/member.
>>>
>> Ok, I keep his in mind for the next patches.
> and can someone please run the unit tests if we are touching this:
>
> /testutil/Invalid Conversions: **
> ERROR:unit/test-util.c:362:test_invalid: assertion failed: (res == NULL)
> Aborted (core dumped)

The purpose of this test was to check if we were failing to convert an 
invalid GSM extended character.
As this was not the 3GPP recommended behavior, it sounds senseless to 
have it in test_invalid() now.

Denis, should we modify it in checking the character of the main default 
alphabet is returned and add it in test_valid() section?

Kind regards,
Guillaume