Re: [PATCH 1/5] gatppp: Add PPP server extension

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Zhenhua,

>  static enum rcr_result ipcp_rcr(struct pppcp_data *pppcp,
>  					const struct pppcp_packet *packet,
>  					guint8 **new_options, guint16 *new_len)
>  {
>  	struct ppp_option_iter iter;
> +	struct ipcp_data *ipcp = pppcp_get_data(pppcp);
> +	guint32 peer_addr = 0;
> +	guint32 dns1 = 0;
> +	guint32 dns2 = 0;
> +	guint32 nbns1 = 0;
> +	guint32 nbns2 = 0;
> 
>  	ppp_option_iter_init(&iter, packet);
> 
> -	if (ppp_option_iter_next(&iter) == FALSE)
> -		return RCR_ACCEPT;
> +	while (ppp_option_iter_next(&iter) == TRUE) {
> +		const guint8 *data = ppp_option_iter_get_data(&iter);
> +
> +		switch (ppp_option_iter_get_type(&iter)) {
> +		case IP_ADDRESS:
> +			memcpy(&peer_addr, data, 4);
> +			break;
> +		case PRIMARY_DNS_SERVER:
> +			memcpy(&dns1, data, 4);
> +			break;
> +		case SECONDARY_DNS_SERVER:
> +			memcpy(&dns2, data, 4);
> +			break;
> +		case PRIMARY_NBNS_SERVER:
> +			memcpy(&nbns1, data, 4);
> +			break;
> +		case SECONDARY_NBNS_SERVER:
> +			memcpy(&nbns2, data, 4);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	if (peer_addr) {
> +		if (ipcp->peer_addr == 0) {
> +			/* RFC 1332 section 3.3
> +			 * As client, accept the server IP as peer's address
> +			 */
> +			ipcp->peer_addr = peer_addr;
> +
> +			return RCR_ACCEPT;

I'm still confused about this part.  As client we should accept server's IP 
address as long as its non-zero.  Otherwise Conf-Rej the option.

> +		} else if (ipcp->peer_addr == peer_addr && ipcp->dns1 == dns1
> +				&& ipcp->nbns1 == nbns1 && ipcp->nbns2 == nbns2)
> +			/* As server, verify the client's info and then send
> +			 * acknowledgement back
> +			 */
> +			return RCR_ACCEPT;
> +	} else {
> +		/* Client requests server to send IP/DNS/NBNS information in the
> +		 * config options
> +		 */
> +		if (ipcp->peer_addr) {
> +			guint8 *options;
> +			guint16 len;
> +
> +			options = ipcp_generate_peer_config_options(ipcp, &len);
> +			if (!options)
> +				goto reject;
> +
> +			*new_len = len;
> +			*new_options = options;
> +			return RCR_NAK;
> +		}

Here we want to ensure that we nak the client's options for as long as the 
options don't match.  Even ipcp->peer_addr is 0 (e.g. we're allocating 
client's address via DHCP or something)

Overall I think this code path is a little too complicated.  Can you see 
whether you can clean it up a bit?

Regards,
-Denis