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

[Denis Kenzior <denkenz@gmail.com>]

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