Re: [PATCHv2 2/6] gatppp: Add IPv6 CP hooks

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Oleg,

>>> +gboolean g_at_ppp_set_ipv6cp_info(GAtPPP *ppp, gboolean is_server,
>>> +                    const char *local, const char *peer);
>>
>> So for consistency sake I think we should introduce
>> g_at_ppp_set_ipv6_server_info(GAtPPP *ppp, const char *remote_prefix);
>>
>> For the local prefix, I'm leaning towards creating a new set of v6 and
>> dual-stack specific constructors:
>>
>> g_at_ppp_ipv6_new(const char *local_prefix);
>> g_at_ppp_ipv6_server_new(const char *local_prefix);
>> g_at_ppp_ipv4v6_new(const char *local_prefix);
>> g_at_ppp_ipv4v6_server_new(const char *local_ip, const char
>> *local_prefix);
>>
>> The key difference between constructors being whether to create ipcp,
>> ipv6cp or both.  We might need a _full version of the server
>> constructors as well.  I'm not entirely happy with this, so if you have
>> better ideas, I'd love to hear them.
> 
> That looks OK to me. Regarding alternatives, one would be:
> 
> enum ofono_ppp_proto {
>     OFONO_PPP_PROTO_IP     = 0x1,
>     OFONO_PPP_PROTO_IPV6   = 0x2,
>     OFONO_PPP_PROTO_IPV4V6 = 0x4,
> };
> 
> struct _GAtPPP {
>     [...]
>     guint8 proto;
> };
> 
> g_at_ppp_new(guint8 proto);
> 
> or/and
> 
> g_at_ppp_set_proto(GAtPPP *ppp, guint8 proto, ...);
> 
> The usage would be:
> 
> - IPv4 only:
> 
>     g_at_ppp_new(OFONO_GPRS_PROTO_IP);
> 
> - IPv6 only:
> 
>     g_at_ppp_new(OFONO_GPRS_PROTO_IPV6);
> 
> - IPv4v6, both optional, but at least one is mandatory:
> 
>     g_at_ppp_new(OFONO_GPRS_PROTO_IPV4V6);
> 
> - IPv4v6, both mandatory:
> 
>     g_at_ppp_new(OFONO_GPRS_PROTO_IPV4V6 |
>          OFONO_GPRS_PROTO_IP | OFONO_GPRS_PROTO_IPV6);
> 
> - IPv4v6, IPv4 mandatory:
> 
>     g_at_ppp_new(OFONO_GPRS_PROTO_IPV4V6 | OFONO_GPRS_PROTO_IP);
> 
> and so on.
> 
> For g_at_ppp_set_proto(GAtPPP *ppp, guint8 proto, ...) there might be
> optional parameters (one IPv4/IPv6 local address or both IPv4 and IPv6
> local addresses), depending on the need. And similarly for
> g_at_ppp_set_server_info().
> 

There are some interesting ideas here, but I want to keep the API
changes and the impact on existing code minimal for now.  This means not
touching the original API if possible.

Besides, we simply don't know if we need all this flexibility, so lets
just consider the ipv4 only, ipv6 only and both ipv4 and ipv6 being
required as use cases we want to address.

Regards,
-Denis