[OpenWrt-Devel] [PATCH 1/1] use NTP server received via DHCP

[Amine Aouled Hamed]

>
> please make var 'iface' local and while you are there, move
> all the 'local' declaration to the head of the function.

Wouldn't be better if the local vars inside of the if and for statements
stay inside?
I mean what is the point of declaring ifaces if I will be using all the
ifaces? in term of performance(even if it is negligible and clarity of the
code).

Thanks for following the patch!
Regards,
Amine

On Thu, Jan 14, 2016 at 10:44 AM, Bastian Bittorf <bittorf at bluebottle.com>
wrote:

> * amine ahd <amine.ahd at gmail.com> [14.01.2016 10:29]:
>
> thank you, patch applies...
>
> >  start_service() {
> > -     local server enabled enable_server peer
> > +     local server enabled enable_server peer ntpservers
> > +     local use_dhcp="$(uci -q get system.ntp.use_dhcp)"
> >
> >       validate_ntp_section ntp || {
> >               echo "validation failed"
> > @@ -21,13 +25,33 @@ start_service() {
> >       }
> >
> >       [ $enabled = 0 ] && return
> > -
> > -     [ -z "$server" ] && return
> > +     [ -z "$server" ] && [ "$use_dhcp" = 0 ] && return
>
> i'am ok with this, if you like you can reuse
> 'config_get_bool()' from /lib/functions.sh
>
> >       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 dhcp_ifaces="$(uci -q get system.ntp.dhcp_ifaces)"
> > +     [ "$use_dhcp" = 1 ] && {
>
> this should also be 'bool'
>
> > +             if [ -z "$dhcp_ifaces" ]; then
> > +                     local dump="$(ubus call network.interface dump)"
> > +                     ntpservers=$(jsonfilter -s "$dump" -e
> '$["interface"][*]["data"]["ntpserver"]')
> > +             else
> > +                     for iface in $dhcp_ifaces; do
>
> please make var 'iface' local and while you are there, move
> all the 'local' declaration to the head of the function.
>
> the rest looks OK to me. - thank you
>
> bye, bastian
>



-- 

Amine Hamed | Software Engineer



Ocedo GmbH | Hirschstrasse 7 | 76133 Karlsruhe | Germany

Email ahamed at ocedo.com


<ahamed at ocedo.com>

REGISTERED OFFICE: KARLSRUHE | DISTRICT COURT: MANNHEIM | REGISTER NUMBER:
HRB 717873
MANAGING DIRECTOR: MARKUS HENNIG|JAN HICHERT
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20160115/ceefa5f5/attachment.htm>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel