| Message ID | 1450800035-15465-1-git-send-email-amine.ahd@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
* amine ahd <amine.ahd@gmail.com> [22.12.2015 17:40]: > + #get the list of ntp servers from DHCP using ubus. > + ntpservers=`ubus call network.interface dump | grep "ntpserver" | cut -d":" -f2 | tr -d '"'` remove the comment, it's obvious what you are doing. when using comment, use a space e.g. # mycomment when speaking with ubus/parsing json do this: ubus list network.interface -> interfaces . /usr/share/libubox/jshn.sh json_load "$( ubus call network.interface.wan2 status )" ... @jow: can you say more about that? > validate_ntp_section ntp || { > echo "validation failed" > return 1 > @@ -22,12 +24,20 @@ start_service() { > > [ $enabled = 0 ] && return > > - [ -z "$server" ] && return > + [ -z "$server" ] && [ "$ntpservers" == "" ] && return please do [ -z "$ntpservers" ] > +handle_default_ntp_servers() { > + local server="$1" > + # append the server to the list > + new_ntp_servers="$new_ntp_servers $server" > +} this comment also does not help 8-) > +local proto=`uci get network.$INTERFACE.proto` please use OpenWrt-style: "$(...)" > +#get the list of ntp servers returned from DHCP, remote leading and trailing whitespaces as well as string quotes > +dhcp_ntp_servers=`ubus call network.interface dump | grep "ntpserver" | cut -d":" -f2 | sed 's/\"//g;s/^[ \t]*//;s/[ \t]*$//'` same as on top, dont parse JSON like this. > +#get the current list of ntp servers in the running instance > +current_ntp_servers=`ubus call service get_data '{"name":"sysntpd"}' | grep "ntp_servers" | cut -d":" -f2 | sed 's/\"//g;s/^[ \t]*//;s/[ \t]*$//'` same as on top bye, bastian
Hi, amine On 23 December 2015 at 00:00, amine ahd <amine.ahd@gmail.com> wrote: > The current state of NTP is to load the list of NTP servers > from the static file /etc/config/system. > This patch allows ntpd to get NTP servers from DHCP. > ntpd will restart whenever the list of NTP servers is changed. > > Signed-off-by: amine hamed <amine.ahd@gmail.com> > --- > package/utils/busybox/Makefile | 3 +++ > package/utils/busybox/files/sysntpd | 15 +++++++++-- > .../package/utils/busybox/files/sysntpd.hotplug | 31 ++++++++++++++++++++++ > 3 files changed, 47 insertions(+), 2 deletions(-) > create mode 100755 package/utils/busybox/files/sysntpd.hotplug > > diff --git a/package/utils/busybox/Makefile b/package/utils/busybox/Makefile > index 9571d48..3c33246 100644 > --- a/package/utils/busybox/Makefile > +++ b/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/package/utils/busybox/files/sysntpd b/package/utils/busybox/files/sysntpd > index f73bb83..fbe1838 100755 > --- a/package/utils/busybox/files/sysntpd > +++ b/package/utils/busybox/files/sysntpd > @@ -1,12 +1,12 @@ > #!/bin/sh /etc/rc.common > # Copyright (C) 2011 OpenWrt.org > > +. /lib/functions.sh > START=98 > > USE_PROCD=1 > PROG=/usr/sbin/ntpd > HOTPLUG_SCRIPT=/usr/sbin/ntpd-hotplug > - > validate_ntp_section() { > uci_validate_section system timeserver "${1}" \ > 'server:list(host)' 'enabled:bool:1' 'enable_server:bool:0' > @@ -15,6 +15,8 @@ validate_ntp_section() { > start_service() { > local server enabled enable_server peer > > + #get the list of ntp servers from DHCP using ubus. > + ntpservers=`ubus call network.interface dump | grep "ntpserver" | cut -d":" -f2 | tr -d '"'` This can be done with help from jsonfilter ubus call network.interface dump | jsonfilter -e '$["interface"][*]["data"]["ntpserver"]' It should also be possible for users to specify sources of timeserver settings, whether it be static list of servers in uci configs or dhcp settings from some network interfaces. > validate_ntp_section ntp || { > echo "validation failed" > return 1 > @@ -22,12 +24,20 @@ start_service() { > > [ $enabled = 0 ] && return > > - [ -z "$server" ] && return > + [ -z "$server" ] && [ "$ntpservers" == "" ] && 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" > + > + #add this data so we can use it in the sysntpd hotplug script. > + procd_set_param data ntp_servers="$ntpservers $server" > + Not quite sure about this, but are we going to replace uci_set_state with procd data param? Personally I prefer uci state for such "dynamic" data store Regards, yousong > + for ntpserver in $ntpservers; do > + procd_append_param command -p $ntpserver > + done > + > for peer in $server; do > procd_append_param command -p $peer > done > @@ -38,5 +48,6 @@ start_service() { > service_triggers() > { > procd_add_reload_trigger "system" > + > procd_add_validation validate_ntp_section > } > diff --git a/package/utils/busybox/files/sysntpd.hotplug b/package/utils/busybox/files/sysntpd.hotplug > new file mode 100755 > index 0000000..de2946a > --- /dev/null > +++ b/package/utils/busybox/files/sysntpd.hotplug > @@ -0,0 +1,31 @@ > +#!/bin/sh > + > +. /lib/functions.sh > + > +[ "$ACTION" = ifup ] || exit 0 > + > +handle_default_ntp_servers() { > + local server="$1" > + # append the server to the list > + new_ntp_servers="$new_ntp_servers $server" > +} > + > +local proto=`uci get network.$INTERFACE.proto` > + > +#get the list of ntp servers returned from DHCP, remote leading and trailing whitespaces as well as string quotes > +dhcp_ntp_servers=`ubus call network.interface dump | grep "ntpserver" | cut -d":" -f2 | sed 's/\"//g;s/^[ \t]*//;s/[ \t]*$//'` > + > +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 get_data '{"name":"sysntpd"}' | grep "ntp_servers" | cut -d":" -f2 | sed 's/\"//g;s/^[ \t]*//;s/[ \t]*$//'` > + > +#if its an up action, the iface uses DHCP and the new list of ntp servers is different from the old, restart sysntpd > +[ "$proto" == "dhcp" ] && [ "$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 > \ No newline at end of file > -- > 2.5.0 > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
Hi, Can you elaborate more on why you prefer uci state? I am just starting with OpenWRT and the first thing I found was procd. Regards, Amine. On Thu, Dec 24, 2015 at 2:35 PM, Yousong Zhou <yszhou4tech@gmail.com> wrote: > Hi, amine > > On 23 December 2015 at 00:00, amine ahd <amine.ahd@gmail.com> wrote: > > The current state of NTP is to load the list of NTP servers > > from the static file /etc/config/system. > > This patch allows ntpd to get NTP servers from DHCP. > > ntpd will restart whenever the list of NTP servers is changed. > > > > Signed-off-by: amine hamed <amine.ahd@gmail.com> > > --- > > package/utils/busybox/Makefile | 3 +++ > > package/utils/busybox/files/sysntpd | 15 +++++++++-- > > .../package/utils/busybox/files/sysntpd.hotplug | 31 > ++++++++++++++++++++++ > > 3 files changed, 47 insertions(+), 2 deletions(-) > > create mode 100755 package/utils/busybox/files/sysntpd.hotplug > > > > diff --git a/package/utils/busybox/Makefile > b/package/utils/busybox/Makefile > > index 9571d48..3c33246 100644 > > --- a/package/utils/busybox/Makefile > > +++ b/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/package/utils/busybox/files/sysntpd > b/package/utils/busybox/files/sysntpd > > index f73bb83..fbe1838 100755 > > --- a/package/utils/busybox/files/sysntpd > > +++ b/package/utils/busybox/files/sysntpd > > @@ -1,12 +1,12 @@ > > #!/bin/sh /etc/rc.common > > # Copyright (C) 2011 OpenWrt.org > > > > +. /lib/functions.sh > > START=98 > > > > USE_PROCD=1 > > PROG=/usr/sbin/ntpd > > HOTPLUG_SCRIPT=/usr/sbin/ntpd-hotplug > > - > > validate_ntp_section() { > > uci_validate_section system timeserver "${1}" \ > > 'server:list(host)' 'enabled:bool:1' > 'enable_server:bool:0' > > @@ -15,6 +15,8 @@ validate_ntp_section() { > > start_service() { > > local server enabled enable_server peer > > > > + #get the list of ntp servers from DHCP using ubus. > > + ntpservers=`ubus call network.interface dump | grep "ntpserver" > | cut -d":" -f2 | tr -d '"'` > > This can be done with help from jsonfilter > > ubus call network.interface dump | jsonfilter -e > '$["interface"][*]["data"]["ntpserver"]' > > It should also be possible for users to specify sources of timeserver > settings, whether it be static list of servers in uci configs or dhcp > settings from some network interfaces. > > > validate_ntp_section ntp || { > > echo "validation failed" > > return 1 > > @@ -22,12 +24,20 @@ start_service() { > > > > [ $enabled = 0 ] && return > > > > - [ -z "$server" ] && return > > + [ -z "$server" ] && [ "$ntpservers" == "" ] && 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" > > + > > + #add this data so we can use it in the sysntpd hotplug script. > > + procd_set_param data ntp_servers="$ntpservers $server" > > + > > Not quite sure about this, but are we going to replace uci_set_state > with procd data param? Personally I prefer uci state for such > "dynamic" data store > > Regards, > > yousong > > > + for ntpserver in $ntpservers; do > > + procd_append_param command -p $ntpserver > > + done > > + > > for peer in $server; do > > procd_append_param command -p $peer > > done > > @@ -38,5 +48,6 @@ start_service() { > > service_triggers() > > { > > procd_add_reload_trigger "system" > > + > > procd_add_validation validate_ntp_section > > } > > diff --git a/package/utils/busybox/files/sysntpd.hotplug > b/package/utils/busybox/files/sysntpd.hotplug > > new file mode 100755 > > index 0000000..de2946a > > --- /dev/null > > +++ b/package/utils/busybox/files/sysntpd.hotplug > > @@ -0,0 +1,31 @@ > > +#!/bin/sh > > + > > +. /lib/functions.sh > > + > > +[ "$ACTION" = ifup ] || exit 0 > > + > > +handle_default_ntp_servers() { > > + local server="$1" > > + # append the server to the list > > + new_ntp_servers="$new_ntp_servers $server" > > +} > > + > > +local proto=`uci get network.$INTERFACE.proto` > > + > > +#get the list of ntp servers returned from DHCP, remote leading and > trailing whitespaces as well as string quotes > > +dhcp_ntp_servers=`ubus call network.interface dump | grep "ntpserver" | > cut -d":" -f2 | sed 's/\"//g;s/^[ \t]*//;s/[ \t]*$//'` > > + > > +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 get_data '{"name":"sysntpd"}' | > grep "ntp_servers" | cut -d":" -f2 | sed 's/\"//g;s/^[ \t]*//;s/[ \t]*$//'` > > + > > +#if its an up action, the iface uses DHCP and the new list of ntp > servers is different from the old, restart sysntpd > > +[ "$proto" == "dhcp" ] && [ "$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 > > \ No newline at end of file > > -- > > 2.5.0 > > _______________________________________________ > > openwrt-devel mailing list > > openwrt-devel@lists.openwrt.org > > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel >
Hi, On 30 December 2015 at 20:12, Amine Aouled Hamed <amine.ahd@gmail.com> wrote: > > Hi, > Can you elaborate more on why you prefer uci state? I am just starting with OpenWRT and the first thing I found was procd. > Most of it is really just personal preference. I prefer uci because it can provide more structured access/manipulation of the state, e.g. - disable those ntpservers on interface down - when it comes to ntpservers from multiple dhcp interfaces - the state can be easily checked with uci command if something went wrong These can of course also be done with procd data, but uci is already there... Regards, yousong
Happy new year! Thanks for the explanation, I will be sending a new patch today. Regards, Amine. On Wed, Dec 30, 2015 at 2:14 PM, Yousong Zhou <yszhou4tech@gmail.com> wrote: > Hi, > > On 30 December 2015 at 20:12, Amine Aouled Hamed <amine.ahd@gmail.com> > wrote: > > > > Hi, > > Can you elaborate more on why you prefer uci state? I am just starting > with OpenWRT and the first thing I found was procd. > > > > Most of it is really just personal preference. I prefer uci because > it can provide more structured access/manipulation of the state, e.g. > > - disable those ntpservers on interface down > - when it comes to ntpservers from multiple dhcp interfaces > - the state can be easily checked with uci command if something went wrong > > These can of course also be done with procd data, but uci is already > there... > > Regards, > yousong >
Hi, please do not use uci state for new features, we consider uci state variables to be deprecated. ~ Jow
On 2015-12-30 14:14, Yousong Zhou wrote: > Hi, > > On 30 December 2015 at 20:12, Amine Aouled Hamed <amine.ahd@gmail.com> wrote: >> >> Hi, >> Can you elaborate more on why you prefer uci state? I am just starting with OpenWRT and the first thing I found was procd. >> > > Most of it is really just personal preference. I prefer uci because > it can provide more structured access/manipulation of the state, e.g. > > - disable those ntpservers on interface down > - when it comes to ntpservers from multiple dhcp interfaces > - the state can be easily checked with uci command if something went wrong > > These can of course also be done with procd data, but uci is already there... Here's some context for why we stopped using uci state: Mixing configuration with a state overlay can easily get very messy, especially when the config changes - it's just too easy for namespace collisions to creep in. Cleanup is also difficult when there's no clear separation of which script is responsible for which part of the state overlay. Using procd data has the advantange that the state is tied to the running state of the service. If the service dies or is restarted, its state is automatically cleaned up too. - Felix
diff --git a/package/utils/busybox/Makefile b/package/utils/busybox/Makefile index 9571d48..3c33246 100644 --- a/package/utils/busybox/Makefile +++ b/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/package/utils/busybox/files/sysntpd b/package/utils/busybox/files/sysntpd index f73bb83..fbe1838 100755 --- a/package/utils/busybox/files/sysntpd +++ b/package/utils/busybox/files/sysntpd @@ -1,12 +1,12 @@ #!/bin/sh /etc/rc.common # Copyright (C) 2011 OpenWrt.org +. /lib/functions.sh START=98 USE_PROCD=1 PROG=/usr/sbin/ntpd HOTPLUG_SCRIPT=/usr/sbin/ntpd-hotplug - validate_ntp_section() { uci_validate_section system timeserver "${1}" \ 'server:list(host)' 'enabled:bool:1' 'enable_server:bool:0' @@ -15,6 +15,8 @@ validate_ntp_section() { start_service() { local server enabled enable_server peer + #get the list of ntp servers from DHCP using ubus. + ntpservers=`ubus call network.interface dump | grep "ntpserver" | cut -d":" -f2 | tr -d '"'` validate_ntp_section ntp || { echo "validation failed" return 1 @@ -22,12 +24,20 @@ start_service() { [ $enabled = 0 ] && return - [ -z "$server" ] && return + [ -z "$server" ] && [ "$ntpservers" == "" ] && 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" + + #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 + for peer in $server; do procd_append_param command -p $peer done @@ -38,5 +48,6 @@ start_service() { service_triggers() { procd_add_reload_trigger "system" + procd_add_validation validate_ntp_section } diff --git a/package/utils/busybox/files/sysntpd.hotplug b/package/utils/busybox/files/sysntpd.hotplug new file mode 100755 index 0000000..de2946a --- /dev/null +++ b/package/utils/busybox/files/sysntpd.hotplug @@ -0,0 +1,31 @@ +#!/bin/sh + +. /lib/functions.sh + +[ "$ACTION" = ifup ] || exit 0 + +handle_default_ntp_servers() { + local server="$1" + # append the server to the list + new_ntp_servers="$new_ntp_servers $server" +} + +local proto=`uci get network.$INTERFACE.proto` + +#get the list of ntp servers returned from DHCP, remote leading and trailing whitespaces as well as string quotes +dhcp_ntp_servers=`ubus call network.interface dump | grep "ntpserver" | cut -d":" -f2 | sed 's/\"//g;s/^[ \t]*//;s/[ \t]*$//'` + +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 get_data '{"name":"sysntpd"}' | grep "ntp_servers" | cut -d":" -f2 | sed 's/\"//g;s/^[ \t]*//;s/[ \t]*$//'` + +#if its an up action, the iface uses DHCP and the new list of ntp servers is different from the old, restart sysntpd +[ "$proto" == "dhcp" ] && [ "$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 \ No newline at end of file
The current state of NTP is to load the list of NTP servers from the static file /etc/config/system. This patch allows ntpd to get NTP servers from DHCP. ntpd will restart whenever the list of NTP servers is changed. Signed-off-by: amine hamed <amine.ahd@gmail.com> --- package/utils/busybox/Makefile | 3 +++ package/utils/busybox/files/sysntpd | 15 +++++++++-- .../package/utils/busybox/files/sysntpd.hotplug | 31 ++++++++++++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-) create mode 100755 package/utils/busybox/files/sysntpd.hotplug