| Message ID | 20240714003458.11832-1-roman@advem.lv |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] netifd: add support for dhcp fallback to static ip | expand |
On 14.07.24 02:34, Roman Yeryomin wrote: > It is pretty common use case for a network device to be configured > as DHCP client but having some fallback static IP address where it > would be reachable for, e.g., configuration. > > This reacts on udhcpc events and sets/removes ipaddr which is > configured on an iface from network config. > This means that for an interface with proto set to dhcp but with > fallback static IP disabled (ipaddr parameter is not set in config) > cost of this feature is one uci call. > If static fallback is enabled (ipaddr is present) then the cost is > 3 uci calls and one ip call for each deconfig/renew/bound udhcpc > event. > It is not nothing but to me it seem like it's a very small overhead > for such a convenience and I think it should be OpenWrt default at > least for targets with one ethernet port. > > Signed-off-by: Roman Yeryomin <roman@advem.lv> > --- > diff --git a/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh b/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh > new file mode 100644 > index 0000000000..79984e9a2f > --- /dev/null > +++ b/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh > @@ -0,0 +1,23 @@ > +#!/bin/sh > + > +[ -z "$1" ] && echo "Error: should be run by udhcpc" && exit 1 > + > +ipaddr=$(uci get network.$INTERFACE.ipaddr) > +[ -z "$ipaddr" ] && exit 0 > + > +ifname=$(uci get network.$INTERFACE.ifname) > +[ -z "$ifname" ] && exit 0 > + > +netmask=$(uci get network.$INTERFACE.netmask) > +[ -z "$netmask" ] && netmask=24 > + > +case "$1" in > + deconfig) > + ip ad add $ipaddr/$netmask dev $ifname > + ;; > + renew|bound) > + ip ad del $ipaddr dev $ifname > + ;; > +esac > + > +exit 0 Here's my take on this patch: 1. The feature should be opt-in via an extra config option, since it changes the behavior in a meaningful way that might be problematic for some users. 2. IP addresses should be set from within netifd, e.g. via the same mechanism that the dhcp script uses to bring up the interface. 3. IP settings should not be read directly from uci, but passed down from the netifd proto handler, e.g. via env vars. - Felix
On 2024-07-14 10:42, Felix Fietkau wrote: > On 14.07.24 02:34, Roman Yeryomin wrote: >> It is pretty common use case for a network device to be configured >> as DHCP client but having some fallback static IP address where it >> would be reachable for, e.g., configuration. >> >> This reacts on udhcpc events and sets/removes ipaddr which is >> configured on an iface from network config. >> This means that for an interface with proto set to dhcp but with >> fallback static IP disabled (ipaddr parameter is not set in config) >> cost of this feature is one uci call. >> If static fallback is enabled (ipaddr is present) then the cost is >> 3 uci calls and one ip call for each deconfig/renew/bound udhcpc >> event. >> It is not nothing but to me it seem like it's a very small overhead >> for such a convenience and I think it should be OpenWrt default at >> least for targets with one ethernet port. >> >> Signed-off-by: Roman Yeryomin <roman@advem.lv> >> --- >> diff --git >> a/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh >> b/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh >> new file mode 100644 >> index 0000000000..79984e9a2f >> --- /dev/null >> +++ >> b/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh >> @@ -0,0 +1,23 @@ >> +#!/bin/sh >> + >> +[ -z "$1" ] && echo "Error: should be run by udhcpc" && exit 1 >> + >> +ipaddr=$(uci get network.$INTERFACE.ipaddr) >> +[ -z "$ipaddr" ] && exit 0 >> + >> +ifname=$(uci get network.$INTERFACE.ifname) >> +[ -z "$ifname" ] && exit 0 >> + >> +netmask=$(uci get network.$INTERFACE.netmask) >> +[ -z "$netmask" ] && netmask=24 >> + >> +case "$1" in >> + deconfig) >> + ip ad add $ipaddr/$netmask dev $ifname >> + ;; >> + renew|bound) >> + ip ad del $ipaddr dev $ifname >> + ;; >> +esac >> + >> +exit 0 > > Here's my take on this patch: > > 1. The feature should be opt-in via an extra config option, since it > changes the behavior in a meaningful way that might be problematic for > some users. > 2. IP addresses should be set from within netifd, e.g. via the same > mechanism that the dhcp script uses to bring up the interface. > 3. IP settings should not be read directly from uci, but passed down > from the netifd proto handler, e.g. via env vars. > Agree on 2. and 3. Can't say I agree on 1. -- could you please give an example in which situation it could be problematic? Thanks for feedback! Regards, Roman
On 14.07.24 11:47, Roman Yeryomin wrote: > On 2024-07-14 10:42, Felix Fietkau wrote: >> On 14.07.24 02:34, Roman Yeryomin wrote: >>> It is pretty common use case for a network device to be configured >>> as DHCP client but having some fallback static IP address where it >>> would be reachable for, e.g., configuration. >>> >>> This reacts on udhcpc events and sets/removes ipaddr which is >>> configured on an iface from network config. >>> This means that for an interface with proto set to dhcp but with >>> fallback static IP disabled (ipaddr parameter is not set in config) >>> cost of this feature is one uci call. >>> If static fallback is enabled (ipaddr is present) then the cost is >>> 3 uci calls and one ip call for each deconfig/renew/bound udhcpc >>> event. >>> It is not nothing but to me it seem like it's a very small overhead >>> for such a convenience and I think it should be OpenWrt default at >>> least for targets with one ethernet port. >>> >>> Signed-off-by: Roman Yeryomin <roman@advem.lv> >>> --- >>> diff --git >>> a/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh >>> b/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh >>> new file mode 100644 >>> index 0000000000..79984e9a2f >>> --- /dev/null >>> +++ >>> b/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh >>> @@ -0,0 +1,23 @@ >>> +#!/bin/sh >>> + >>> +[ -z "$1" ] && echo "Error: should be run by udhcpc" && exit 1 >>> + >>> +ipaddr=$(uci get network.$INTERFACE.ipaddr) >>> +[ -z "$ipaddr" ] && exit 0 >>> + >>> +ifname=$(uci get network.$INTERFACE.ifname) >>> +[ -z "$ifname" ] && exit 0 >>> + >>> +netmask=$(uci get network.$INTERFACE.netmask) >>> +[ -z "$netmask" ] && netmask=24 >>> + >>> +case "$1" in >>> + deconfig) >>> + ip ad add $ipaddr/$netmask dev $ifname >>> + ;; >>> + renew|bound) >>> + ip ad del $ipaddr dev $ifname >>> + ;; >>> +esac >>> + >>> +exit 0 >> >> Here's my take on this patch: >> >> 1. The feature should be opt-in via an extra config option, since it >> changes the behavior in a meaningful way that might be problematic for >> some users. >> 2. IP addresses should be set from within netifd, e.g. via the same >> mechanism that the dhcp script uses to bring up the interface. >> 3. IP settings should not be read directly from uci, but passed down >> from the netifd proto handler, e.g. via env vars. >> > > Agree on 2. and 3. > Can't say I agree on 1. -- could you please give an example in which > situation it could be problematic? If you use the ipaddr to request a specific address from the DHCP server, but don't want the link to be up when the server is not reachable. - Felix
On 2024-07-14 14:11, Felix Fietkau wrote: > On 14.07.24 11:47, Roman Yeryomin wrote: >> On 2024-07-14 10:42, Felix Fietkau wrote: >>> On 14.07.24 02:34, Roman Yeryomin wrote: >>>> It is pretty common use case for a network device to be configured >>>> as DHCP client but having some fallback static IP address where it >>>> would be reachable for, e.g., configuration. >>>> >>>> This reacts on udhcpc events and sets/removes ipaddr which is >>>> configured on an iface from network config. >>>> This means that for an interface with proto set to dhcp but with >>>> fallback static IP disabled (ipaddr parameter is not set in config) >>>> cost of this feature is one uci call. >>>> If static fallback is enabled (ipaddr is present) then the cost is >>>> 3 uci calls and one ip call for each deconfig/renew/bound udhcpc >>>> event. >>>> It is not nothing but to me it seem like it's a very small overhead >>>> for such a convenience and I think it should be OpenWrt default at >>>> least for targets with one ethernet port. >>>> >>>> Signed-off-by: Roman Yeryomin <roman@advem.lv> >>>> --- >>>> diff --git >>>> a/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh >>>> b/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh >>>> new file mode 100644 >>>> index 0000000000..79984e9a2f >>>> --- /dev/null >>>> +++ >>>> b/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh >>>> @@ -0,0 +1,23 @@ >>>> +#!/bin/sh >>>> + >>>> +[ -z "$1" ] && echo "Error: should be run by udhcpc" && exit 1 >>>> + >>>> +ipaddr=$(uci get network.$INTERFACE.ipaddr) >>>> +[ -z "$ipaddr" ] && exit 0 >>>> + >>>> +ifname=$(uci get network.$INTERFACE.ifname) >>>> +[ -z "$ifname" ] && exit 0 >>>> + >>>> +netmask=$(uci get network.$INTERFACE.netmask) >>>> +[ -z "$netmask" ] && netmask=24 >>>> + >>>> +case "$1" in >>>> + deconfig) >>>> + ip ad add $ipaddr/$netmask dev $ifname >>>> + ;; >>>> + renew|bound) >>>> + ip ad del $ipaddr dev $ifname >>>> + ;; >>>> +esac >>>> + >>>> +exit 0 >>> >>> Here's my take on this patch: >>> >>> 1. The feature should be opt-in via an extra config option, since it >>> changes the behavior in a meaningful way that might be problematic >>> for >>> some users. >>> 2. IP addresses should be set from within netifd, e.g. via the same >>> mechanism that the dhcp script uses to bring up the interface. >>> 3. IP settings should not be read directly from uci, but passed down >>> from the netifd proto handler, e.g. via env vars. >>> >> >> Agree on 2. and 3. >> Can't say I agree on 1. -- could you please give an example in which >> situation it could be problematic? > > If you use the ipaddr to request a specific address from the DHCP > server, but don't want the link to be up when the server is not > reachable. Ah, got it, let me send updated version then with 'fallbackip' option. Regards, Roman
diff --git a/package/network/config/netifd/Makefile b/package/network/config/netifd/Makefile index d80c2eeed6..8ef391d913 100644 --- a/package/network/config/netifd/Makefile +++ b/package/network/config/netifd/Makefile @@ -42,7 +42,6 @@ define Package/netifd/install $(INSTALL_DIR) $(1)/sbin $(INSTALL_BIN) $(PKG_BUILD_DIR)/netifd $(1)/sbin/ $(CP) ./files/* $(1)/ - $(INSTALL_DIR) $(1)/etc/udhcpc.user.d/ $(CP) \ $(PKG_BUILD_DIR)/scripts/utils.sh \ $(PKG_BUILD_DIR)/scripts/netifd-proto.sh \ diff --git a/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh b/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh new file mode 100644 index 0000000000..79984e9a2f --- /dev/null +++ b/package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh @@ -0,0 +1,23 @@ +#!/bin/sh + +[ -z "$1" ] && echo "Error: should be run by udhcpc" && exit 1 + +ipaddr=$(uci get network.$INTERFACE.ipaddr) +[ -z "$ipaddr" ] && exit 0 + +ifname=$(uci get network.$INTERFACE.ifname) +[ -z "$ifname" ] && exit 0 + +netmask=$(uci get network.$INTERFACE.netmask) +[ -z "$netmask" ] && netmask=24 + +case "$1" in + deconfig) + ip ad add $ipaddr/$netmask dev $ifname + ;; + renew|bound) + ip ad del $ipaddr dev $ifname + ;; +esac + +exit 0
It is pretty common use case for a network device to be configured as DHCP client but having some fallback static IP address where it would be reachable for, e.g., configuration. This reacts on udhcpc events and sets/removes ipaddr which is configured on an iface from network config. This means that for an interface with proto set to dhcp but with fallback static IP disabled (ipaddr parameter is not set in config) cost of this feature is one uci call. If static fallback is enabled (ipaddr is present) then the cost is 3 uci calls and one ip call for each deconfig/renew/bound udhcpc event. It is not nothing but to me it seem like it's a very small overhead for such a convenience and I think it should be OpenWrt default at least for targets with one ethernet port. Signed-off-by: Roman Yeryomin <roman@advem.lv> --- package/network/config/netifd/Makefile | 1 - .../files/etc/udhcpc.user.d/fallbackip.sh | 23 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 package/network/config/netifd/files/etc/udhcpc.user.d/fallbackip.sh