Re: [PATCH 03/11] gatppp: Add PPP server extension

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Zhenhua,

> >> +void ipcp_set_server_info(struct pppcp_data *pppcp, guint32
> >> local_addr, +				guint32 peer_addr, +				guint32 dns1, guint32 dns2,
> >> +				guint32 nbns1, guint32 nbns2)
> >> +{
> >> +	struct ipcp_data *ipcp = pppcp_get_data(pppcp);
> >> +
> >> +	ipcp->req_options = REQ_OPTION_IPADDR | REQ_OPTION_DNS1 |
> >> +				REQ_OPTION_DNS2 | REQ_OPTION_NBNS1 |
> >> +				REQ_OPTION_NBNS2;
> >> +
> >> +	ipcp->local_addr = local_addr;
> >> +	ipcp->peer_addr = peer_addr;
> >> +	ipcp->dns1 = dns1;
> >> +	ipcp->dns2 = dns2;
> >> +	ipcp->nbns1 = nbns1;
> >> +	ipcp->nbns2 = nbns2;
> >> +
> >> +	ipcp_generate_config_options(ipcp);
> >
> > Please note that this function is currently having no effect,
> > you're missing a
> > call to pppcp_set_local_options.  However, in the case of a server, we
> > actually do not want to request DNS or NBNS, and we should
> > only send our local
> > address to the peer if it is non-zero.  Please note that all
> > PPP modems we
> > have encountered so far, do not send their local IP address in
> > a Configure-
> > Request.  Thus calling ipcp_set_server_info should end up
> > generating an empty
> > Configure-Request or with only the local IP address present.
> 
> It's confused me a little. Please correct me if I am wrong. I think PPP
>  modem should send their local IP address to peer in a Configure-Request.
>  So client could accept the peer address in ipcp_rcr.

This is correct if the PPP server wants to do so, it is perfectly OK for the 
server not to send its IP address (e.g. if it doesn't have one.)  This is what 
we observe on most PPP modems.

> 
> In the case of a server, ipcp_set_server_info() should set local, peer, DNS
>  and NBNS info and call pppcp_set_local_options(). And server only send our
>  local address to peer in a Configure-Request.

I'd modify this slightly.  If ipcp_set_server_info is called with server's 
local address as 0, then don't even send IP-Address in a configure Request.  
This wouldn't make sense, in effect the server is asking the client to provide 
its IP address. Otherwise, go ahead and send it (as the client will simply 
ack).

> 
> >> +}
> >> +
> >>  static void ipcp_reset_config_options(struct ipcp_data *ipcp)  {
> >>  	ipcp->req_options = REQ_OPTION_IPADDR | REQ_OPTION_DNS1 |
> >>  				REQ_OPTION_DNS2 | REQ_OPTION_NBNS1 |
> >>  				REQ_OPTION_NBNS2;
> >> -	ipcp->ipaddr = 0;
> >> +	ipcp->local_addr = 0;
> >
> > In the case of a server, the local_address should not be reset.
> 
> It seems we need to add a flag in ipcp_data to indicate whether it's server
>  or not. Does it make sense?

Might be a good idea.

Regards,
-Denis