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

[Oleg Zhurakivskyy <oleg.zhurakivskyy@intel.com>]

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

Hello Denis,

On 11/15/2011 04:53 AM, Denis Kenzior wrote:
>> +	case IPV6CP_PROTO:
>> +		if (ppp->ipv6cp)
>> +			pppcp_process_packet(ppp->ipv6cp, packet, len - offset);
>> +		break;
>
> Are you sure you don't want to send a protocol reject here?

Sure, will make it so, thanks for the idea.

>> +
>> +	if (ppp->ipv6cp)
>> +		pppcp_signal_close(ppp->ipv6cp);
>
> This also looks fine, but please add a newline here

OK.

>> +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().

Regards,
Oleg
-- 
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki