Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Slava,

On 09/19/2018 03:35 AM, Slava Monich wrote:
> On 19/09/18 08:37, Giacinto Cifelli wrote:
>> ---
>>   include/gprs-context.h |  1 +
>>   include/lte.h          | 11 +++++++++--
>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/gprs-context.h b/include/gprs-context.h
>> index 20ca9ef..8869c12 100644
>> --- a/include/gprs-context.h
>> +++ b/include/gprs-context.h
>> @@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
>>   enum ofono_gprs_auth_method {
>>       OFONO_GPRS_AUTH_METHOD_CHAP = 0,
>>       OFONO_GPRS_AUTH_METHOD_PAP,
>> +    OFONO_GPRS_AUTH_METHOD_NONE,
> 
> I think there should be OFONO_GPRS_AUTH_METHOD_ANY (or 
> OFONO_GPRS_AUTH_METHOD_PAP_CHAP) here as well, for completeness. Many 
> modems support that too (and we had to add it in our fork).

There is no such thing in 3GPP, so how does that work?

And by the way,  none of the provisioning databases I'm aware of have 
this possibility and CHAP is actually a sane default.  While there are 
probably 2 providers left on this planet that might still insist on PAP, 
the 3GPP specs actually mandate CHAP anyway.

> 
> 
>>   };
>>   struct ofono_gprs_primary_context {
>> diff --git a/include/lte.h b/include/lte.h
>> index ef84ab9..38587c3 100644
>> --- a/include/lte.h
>> +++ b/include/lte.h
>> @@ -3,6 +3,7 @@
>>    *  oFono - Open Source Telephony
>>    *
>>    *  Copyright (C) 2016  Endocode AG. All rights reserved.
>> + *  Copyright (C) 2018 Gemalto M2M
>>    *
>>    *  This program is free software; you can redistribute it and/or 
>> modify
>>    *  it under the terms of the GNU General Public License version 2 as
>> @@ -28,14 +29,18 @@ extern "C" {
>>   #include <ofono/types.h>
>> -struct ofono_lte;
>> -
>>   struct ofono_lte_default_attach_info {
>>       char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
>> +    enum ofono_gprs_proto proto;
>> +    enum ofono_gprs_auth_method auth_method;
>> +    char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
>> +    char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
>>   };
> 
> This is starting to look suspiciously similar to struct 
> ofono_gprs_primary_context (the only thing left is cid). Is it really 
> necessary to maintain two copies of essentially the same structure or is 
> there some room for unification here?
> 

I don't really see a problem here.  One is active context details, the 
other is default attach details.  But if OFONO_GPRS_* defines are going 
to be used in multiple include/ofono files, then these defines should be 
moved to types.h.

Regards,
-Denis