| Message ID | 1452158131-28603-1-git-send-email-amine.ahd@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
* amine ahd <amine.ahd@gmail.com> [07.01.2016 10:34]: the patch is from wrong dir. please do a 'git format-patch' inside the OpenWrt-dir, so the modified files are: package/utils/busybox/Makefile package/utils/busybox/files/sysntpd package/utils/busybox/files/sysntpd.hotplug for the subject: what means "[DEV-1329]"? > +. /usr/share/libubox/jshn.sh > START=98 > > USE_PROCD=1 > @@ -22,12 +24,32 @@ start_service() { > > [ $enabled = 0 ] && return > > - [ -z "$server" ] && return please check if any interface is in DHCP-mode and has a chance to get an NTP, otherwise return. > + if [ "$use_dhcp" = 1 ]; then minor: use OpenWrt-style: when there is no 'else', just do: [ "$use_dhcp" = 1 ] && { ... } > + if [ -z "$dhcp_ifaces" ]; then > + dump=$(ubus call network.interface dump) make 'dump' also 'local' > +check_int() { minor: choose better function name. > + list=$(uci get system.ntp.dhcp_ifaces) > + if [ -z $list ]; > + then > + return 0 > + fi it's shorter: [ -z "$list" ] && return > + if [ "${list#*$INTERFACE}" != "$list" ] this looks strange to me and will IMHO not work for similar names, e.g. eth0 eth0.1 eth0.2 you want to test, if the upcoming $INTERFACE is part of allowed interfaces ("system.ntp.dhcp_ifaces"), aren't you? is_valid_interface() { local list="$(uci get system.ntp.dhcp_ifaces)" case " $list " in *" $INTERFACE "*) ;; *) return 1 ;; esac } > + for int in $dhcp_ifaces; do please you 'iface' or 'interface' not 'int' but thanks for the patch for now! bye, bastian
On Thu, Jan 7, 2016 at 10:58 AM, Bastian Bittorf <bittorf@bluebottle.com> wrote: > > the patch is from wrong dir. > please do a 'git format-patch' inside the OpenWrt-dir, > so the modified files are: > > package/utils/busybox/Makefile > package/utils/busybox/files/sysntpd > package/utils/busybox/files/sysntpd.hotplug > > for the subject: what means "[DEV-1329]"? > > please check if any interface is in DHCP-mode > and has a chance to get an NTP, otherwise return. > My mistake I forgot to change the directory when creating the patch! [DEV-1329] is used internally by our company :p question: Why would we need to return if there aren't interface in DHCP mode? the static list is always used and for DHCP it is left to the user to add that option in etc/config/system. I think it is safe to assume if the user chooses to use the DHCP option then there will be at least one interface in DHCP mode? Regards, Amine.
* Amine Aouled Hamed <amine.ahd@gmail.com> [07.01.2016 14:37]: > > please check if any interface is in DHCP-mode > > and has a chance to get an NTP, otherwise return. > > question: Why would we need to return if there aren't interface in DHCP > mode? the static list is always used and for DHCP it is left to the user to > add that option in etc/config/system. I think it is safe to assume if the > user chooses to use the DHCP option then there will be at least one > interface in DHCP mode? when looking at the present code, it returns when the static list is empty. maybe it's a rare condition, but your code changes the behavior. for now ignore this and polish the rest, lets see what the others say. bye, bastian
On Thu, Jan 7, 2016 at 2:42 PM, Bastian Bittorf <bittorf@bluebottle.com> wrote: > when looking at the present code, it > returns when the static list is empty. > > maybe it's a rare condition, but your code > changes the behavior. for now ignore this > and polish the rest, lets see what the others say. > > bye, bastian > AFAIK, this was the default behaviour. I removed it because IMO it is not the case anymore to exit when we have an empty static list. If this needs to be changed, please let me know. Regards, Amine.
diff --git a/openwrt/package/utils/busybox/Makefile b/openwrt/package/utils/busybox/Makefile index 9571d48..3c33246 100644 --- a/openwrt/package/utils/busybox/Makefile +++ b/openwrt/package/utils/busybox/Makefile @@ -112,6 +112,9 @@ define Package/busybox/install $(INSTALL_BIN) ./files/cron $(1)/etc/init.d/cron $(INSTALL_BIN) ./files/telnet $(1)/etc/init.d/telnet $(INSTALL_BIN) ./files/sysntpd $(1)/etc/init.d/sysntpd + $(INSTALL_DIR) $(1)/etc/hotplug.d + $(INSTALL_DIR) $(1)/etc/hotplug.d/iface + $(INSTALL_BIN) ./files/sysntpd.hotplug $(1)/etc/hotplug.d/iface/30-sysntpd $(INSTALL_BIN) ./files/ntpd-hotplug $(1)/usr/sbin/ntpd-hotplug -rm -rf $(1)/lib64 endef diff --git a/openwrt/package/utils/busybox/files/sysntpd b/openwrt/package/utils/busybox/files/sysntpd index f73bb83..dec474c 100755 --- a/openwrt/package/utils/busybox/files/sysntpd +++ b/openwrt/package/utils/busybox/files/sysntpd @@ -1,6 +1,8 @@ #!/bin/sh /etc/rc.common # Copyright (C) 2011 OpenWrt.org +. /lib/functions.sh +. /usr/share/libubox/jshn.sh START=98 USE_PROCD=1 @@ -22,12 +24,32 @@ start_service() { [ $enabled = 0 ] && return - [ -z "$server" ] && return - procd_open_instance procd_set_param command "$PROG" -n [ "$enable_server" = "1" ] && procd_append_param command -l [ -x "$HOTPLUG_SCRIPT" ] && procd_append_param command -S "$HOTPLUG_SCRIPT" + + local use_dhcp="$(uci -q get system.ntp.use_dhcp)" + local dhcp_ifaces="$(uci -q get system.ntp.dhcp_ifaces)" + if [ "$use_dhcp" = 1 ]; then + if [ -z "$dhcp_ifaces" ]; then + dump=$(ubus call network.interface dump) + ntpservers=$(jsonfilter -s "$dump" -e '$["interface"][*]["data"]["ntpserver"]') + else + for int in $dhcp_ifaces; do + status=$(ubus call network.interface.$int status) + ntpserver=$(jsonfilter -s "$status" -e '$["data"]["ntpserver"]') + [ -n "$ntpserver" ] && + ntpservers="$ntpservers $ntpserver" + done + fi + # add this data so we can use it in the sysntpd hotplug script. + procd_set_param data ntp_servers="$ntpservers $server" + for ntpserver in $ntpservers; do + procd_append_param command -p $ntpserver + done + fi + for peer in $server; do procd_append_param command -p $peer done @@ -38,5 +60,7 @@ start_service() { service_triggers() { procd_add_reload_trigger "system" + procd_add_validation validate_ntp_section + } diff --git a/openwrt/package/utils/busybox/files/sysntpd.hotplug b/openwrt/package/utils/busybox/files/sysntpd.hotplug new file mode 100755 index 0000000..a9fae1b --- /dev/null +++ b/openwrt/package/utils/busybox/files/sysntpd.hotplug @@ -0,0 +1,54 @@ +#!/bin/sh + +. /lib/functions.sh +. /usr/share/libubox/jshn.sh + +check_int() { + list=$(uci get system.ntp.dhcp_ifaces) + if [ -z $list ]; + then + return 0 + fi + + if [ "${list#*$INTERFACE}" != "$list" ] + then + return 0 + else + return 1 + fi +} + +local proto="$(uci get network.$INTERFACE.proto)" +local use_dhcp="$(uci -q get system.ntp.use_dhcp)" +[ "$use_dhcp" = 1 ] && [ "$ACTION" = ifup ] && check_int && [ "$proto" = dhcp -o "$proto" = dhcp6 ] || exit 0 + +handle_default_ntp_servers() { + local server="$1" + new_ntp_servers="$new_ntp_servers $server" +} + +local dhcp_ifaces="$(uci -q get system.ntp.dhcp_ifaces)" +if [ -z "$dhcp_ifaces" ]; then + dump=$(ubus call network.interface dump) + dhcp_ntp_servers=$(jsonfilter -s "$dump" -e '$["interface"][*]["data"]["ntpserver"]') +else + for int in $dhcp_ifaces; do + status=$(ubus call network.interface.$int status) + ntpserver=$(jsonfilter -s "$status" -e '$["data"]["ntpserver"]') + [ -n "$ntpserver" ] && + dhcp_ntp_servers="dhcp_ntp_servers $ntpserver" + done +fi + +new_ntp_servers="$dhcp_ntp_servers" +#get the default list of ntp servers from the config file and append it to the new list +config_load system +config_list_foreach "ntp" "server" handle_default_ntp_servers + +#get the current list of ntp servers in the running instance +current_ntp_servers=$(ubus call service list '{"name":"sysntpd", "verbose":true}' | jsonfilter -e '$["sysntpd"]["instances"][*]["data"]["ntp_servers"]') +#if its an up action, the iface uses DHCP and the new list of ntp servers is different from the old, restart sysntpd +[ "$current_ntp_servers" != "$new_ntp_servers" ] || exit 0 + +logger -t sysntpd "Reloading sysntpd due to $ACTION of interface $INTERFACE and a change of NTP servers" +/etc/init.d/sysntpd enabled && /etc/init.d/sysntpd reload