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

[Zhang, Zhenhua]

Hi Denis,

Denis Kenzior wrote:
> 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.

Okay. That's fine. At least, PPP server need to send peer_addr in Conf-Nak reply. Client 'thinks' it's server address.

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

No problems. If server local address is zero, we don't send IP in a configure request.

>> 
>>>> +}
>>>> +
>>>>  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



Regards,
Zhenhua