Re: [PATCH 1/1] gatchat: Add IPv6 CP support

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

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

Hello Denis,

On 10/31/2011 09:43 AM, Denis Kenzior wrote:
> This looks like a good start.  However, please separate this patch into
> several, like this:
>
> - One adding the bulk of IPv6CP protocol
> - One for hooks in gatppp itself&  ipv6cp
> - One for gsmdial
> - One for test-server.

Sure, will structure like that.

> Can IPV6CP and IPCP co-exist?  In theory there is such a concept as
> dual-stack contexts, however I don't know whether any modem in existence
> does this with PPP.

Incidentally, I was thinking about this too. I already made IPCP and IPv6CP to 
co-exist, will post it together with corrections in the next version.

>> +void g_at_ppp_set_ipv6cp_info(GAtPPP *ppp, const char *local, const char *peer)
>> +{
>> +	struct in6_addr local_addr;
>> +	struct in6_addr peer_addr;
>> +
>> +	if (local)
>> +		inet_pton(AF_INET6, local,&local_addr);
>> +	else
>> +		memset(&local_addr, 0, sizeof(local_addr));
>> +
>
> What if inet_pton fails?

Sure, I will improve this.

>> +struct eui64 {
[...]
>> +};
>
> I really don't like this actually.  I would either simply use an
> unsigned char[8] array or use guint64.

Sure, simple guint8[8] will do.

>> +struct pppcp_data *ipv6cp_new(GAtPPP *ppp, const struct eui64 *local_addr,
>> +				const struct eui64 *peer_addr);
>
> Thinking more about it, why don't you make this function take const char
> * arguments instead, and hide away all the nasty details inside
> ipv6cp.c?  This way all these funky structs are hidden away in the
> implementation, and you can use whatever is the most convenient.

Good point, I will correct.

>> +enum ipv6cp_option_types {
>> +	IPV6CP_INTERFACE_ID		= 1,
>> +	IPV6CP_COMPRESSION_PROTO	= 2,
>
> The latest RFC 5072 doesn't even have this option, so you can probably
> get rid of it.

OK.

>> +static enum rcr_result ipv6cp_peer_addr_check(struct ipv6cp_data *ipv6cp,
>> +						const void *data)
>> +{
>> +	if (memcmp(&eui64_any, data, sizeof(eui64_any)) == 0)
>> +		return RCR_NAK;
>> +
>
> If we're playing the server role, then we might want to NAK non-zero
> peer interface ids as well.

OK.

>> +static enum rcr_result ipv6cp_rcr(struct pppcp_data *pppcp,
>> +					const struct pppcp_packet *packet,
>> +					guint8 **new_options, guint16 *new_len)
>
> Is there a particular reason you did not structure this function in the
> same manner as ipcp_server/client_rcr?  They are pretty much optimized
> with no allocations on the fast (non-reject) path and I'd like to have
> consistency in the code.

I tried to simplify this a bit, and to get rid of the client-server separation, 
if possible. I missed the point that the fast path is optimized.

Anyway, thanks for the explanation, I will separate into client and server and 
make sure there aren't extra allocations on the fast path.

>> +static void ipv6cp_rcn_nak(struct pppcp_data *pppcp,
>> +				const struct pppcp_packet *packet)
>> +{
>> +	struct ipv6cp_data *ipv6cp = pppcp_get_data(pppcp);
>> +	struct ppp_option_iter iter;
>> +
>> +	ppp_option_iter_init(&iter, packet);
>> +
>
> We have to be a bit careful here, if we are playing the server role then
> we should not let the client dictate our interface ID.  The reason being
> that e.g. ConnMan might be allocating our interface ids from its own
> pool and we can't really change it at this point.

OK, thanks for the input here, I will take this into account.

>> +	while (ppp_option_iter_next(&iter) == TRUE) {
>> +		const guint8 *data = ppp_option_iter_get_data(&iter);
>> +
>> +		switch (ppp_option_iter_get_type(&iter)) {
>> +		case IPV6CP_INTERFACE_ID:
>> +			memcpy(&ipv6cp->local_addr, data,
>> +				sizeof(ipv6cp->local_addr));
>
> You might also want to make sure to set the flag to request the
> interface id, though this is more of a problem with IPCP.

OK, will check this.

>> +	while (ppp_option_iter_next(&iter) == TRUE) {
>> +		switch (ppp_option_iter_get_type(&iter)) {
>> +		case IPV6CP_INTERFACE_ID:
>> +			ipv6cp->req_options&= ~IPV6CP_INTERFACE_ID;
>> +			memset(&ipv6cp->local_addr, 0,
>> +				sizeof(ipv6cp->local_addr));
>
> This means that the peer refused to configure this option, it doesn't
> mean that our idea of our interface (e.g. if we're the server) should be
> reset...

Will check this too.

>> +	g_print("\ttest-server [-t type] [-6 addr]\n");
>
> Hm, this message makes no sense.

This was added for testing purposes, just in order to test if both ends can 
exchange the IDs. I will remove it, if it's not necessary.

Thanks for the comments, I will prepare and send another set.

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