Re: [lwip-devel] [patch #8361] Add support for NTP option in DHCP

[Claudius Zingerli]

Hi Sergio,

I do support your approach. Just don't call it _S_ntp, as NTP is the 
underlying protocol and DHCP shouldn't decide about which variant to 
use. How about adding a function pointer to the dhcp struct to 
ntp_addserver()? If it's not initialized, the DHCP code just ignores the 
data.
Prototype:
1. void ntp_addserver(ipaddr_t *p_servers, size_t p_servers_count);
or
2. void ntp_addserver(uint8_t *p_servers, size_t p_servers_size);
or
3. void ntp_addserver(uint8_t *p_server_address, size_t 
p_server_address_size);

Then we don't need NTP_MAX_SERVERS anymore, as the DHCP code could
call the 3rd version multiple times and let the ntp implementation 
handle this.
I'm not sure if we are guaranteed to receive IPv4 addresses (I don't 
think so)
Approach 2: just pass a byte array and size and let the implementation 
handle the parsing
Approach 1: Parse it in the DHCP client code. I'm not sure about 
handling sizeof(ip_addr) if we receive something else than expected 
(buggy, malicious or ipv6 instead of ipv4)

I don't think there is any reason to return anything, right?

Regards

Claudius


On 2014-06-13 16:48, Sergio R. Caprile wrote:
> Follow-up Comment #2, patch #8361 (project lwip):
>
> This patch stores NTP server addresses in a local DHCP struct.
> I think we should follow the same operations as for DNS servers:
> * The DHCP server gets the offered servers and calls sntp_addserver() for each
> one.
> ** Pros: the DHCP module does not need to know about SNTP of the amount of
> memory for servers, it just calls the add function.
> ** Cons: the dhcp module must know the prototype for this function and sntp.c
> is not part of the lwIP tree
> * The SNTP module provides a new sntp_addserver() function, similar to the
> current DNS module function.
> * Pros: SNTP_MAX_SERVERS is an SNTP module macro instead of NTP_MAX_SERVERS a
> DHCP module macro.
>
> I can do the changes based on the OP uploaded patch, and submit a new patch,
> if you guys agree.
>
>
>      _______________________________________________________
>
> Reply to this item at:
>
>    <http://savannah.nongnu.org/patch/?8361>
>
> _______________________________________________
>    Message sent via/by Savannah
>    http://savannah.nongnu.org/
>
>
> _______________________________________________
> lwip-devel mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/lwip-devel