* [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices @ 2025-04-07 17:49 Jeroen Hofstee 2025-04-09 8:37 ` Richard Purdie 0 siblings, 1 reply; 11+ messages in thread From: Jeroen Hofstee @ 2025-04-07 17:49 UTC (permalink / raw) To: openembedded-core; +Cc: Jeroen Hofstee From: Jeroen Hofstee <jhofstee@victronenergy.com> Commit 160f713917 (udev-extraconf: fix network.sh script did not configure hotplugged interfaces, 2024-10-18) fixed ifupdown for hotplug devices, but also calls it for non hotplug devices. That can cause issue, since they might not expect the ifupdown from udev, since it wasn't called before mentioned patch got merged. For util-linux this can simply be fixed by adding --allow=hotplug. Unfortunately busybox doesn't have that option, so a function is added to check if the device is marked as allow-hotplug. Since wilcards are supported adding 'allow-hotplug *' allows to restore behaviour of mentioned patch, while this restores the original behaviour. Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com> --- .../udev/udev-extraconf/network.sh | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/meta/recipes-core/udev/udev-extraconf/network.sh b/meta/recipes-core/udev/udev-extraconf/network.sh index 500e60ae61..da79f00d5a 100644 --- a/meta/recipes-core/udev/udev-extraconf/network.sh +++ b/meta/recipes-core/udev/udev-extraconf/network.sh @@ -6,6 +6,34 @@ echo "$INTERFACE" | grep -q wifi && exit 0 # udevd does clearenv(). Export shell PATH to children. export PATH +# udev should only trigger ifupdown for interfaces marked as allow-hotplug +# and with util-linux that is as simple as adding --allow=hotplug. +# Busybox unfortunately doesn't have this option. +# allow-hotplug is a pattern like eth0 /eth* /eth*/1 /eth*=eth. +# This function checks if INTERFACE matches an allow-hotplug pattern. + +allow_hotplug() { + allow_hotplug="$(sed -n -e 's/^allow-hotplug \+\([^= ]*\).*/\1/p' /etc/network/interfaces)" + for pattern in $allow_hotplug; do + options="$(echo $pattern | sed -n -e 's,^/\?[^ /]\+/\(.*\),\1,p')" + value="$(echo $pattern | sed -n -e 's,^/\?\([^ /]\+\).*,\1,p')" + interfaces="$(ls -d /sys/class/net/$value 2>/dev/null | xargs -r -n 1 basename)" + if [ "$options" != "" ]; then + interfaces="$(echo $interfaces | awk -v n=$options '{print $n }')" + fi + echo "$interfaces" | grep -w -q "$INTERFACE" + if [ $? -eq 0 ]; then + return 0 + fi + done + + return 1 +} + +if ! allow_hotplug; then + exit 0 +fi + # if this interface has an entry in /etc/network/interfaces, let ifupdown # handle it if grep -q "iface \+$INTERFACE" /etc/network/interfaces; then -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices 2025-04-07 17:49 [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices Jeroen Hofstee @ 2025-04-09 8:37 ` Richard Purdie 2025-04-09 9:08 ` Jeroen Hofstee 2025-04-09 9:28 ` Mike Looijmans 0 siblings, 2 replies; 11+ messages in thread From: Richard Purdie @ 2025-04-09 8:37 UTC (permalink / raw) To: jeroen, openembedded-core; +Cc: Jeroen Hofstee On Mon, 2025-04-07 at 19:49 +0200, Jeroen Hofstee via lists.openembedded.org wrote: > From: Jeroen Hofstee <jhofstee@victronenergy.com> > > Commit 160f713917 (udev-extraconf: fix network.sh script did not > configure hotplugged interfaces, 2024-10-18) fixed ifupdown for > hotplug devices, but also calls it for non hotplug devices. That > can cause issue, since they might not expect the ifupdown from > udev, since it wasn't called before mentioned patch got merged. > > For util-linux this can simply be fixed by adding --allow=hotplug. > Unfortunately busybox doesn't have that option, so a function is > added to check if the device is marked as allow-hotplug. > > Since wilcards are supported adding 'allow-hotplug *' allows to > restore behaviour of mentioned patch, while this restores the > original behaviour. > > Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com> > --- > .../udev/udev-extraconf/network.sh | 28 +++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/meta/recipes-core/udev/udev-extraconf/network.sh b/meta/recipes-core/udev/udev-extraconf/network.sh > index 500e60ae61..da79f00d5a 100644 > --- a/meta/recipes-core/udev/udev-extraconf/network.sh > +++ b/meta/recipes-core/udev/udev-extraconf/network.sh > @@ -6,6 +6,34 @@ echo "$INTERFACE" | grep -q wifi && exit 0 > # udevd does clearenv(). Export shell PATH to children. > export PATH > > +# udev should only trigger ifupdown for interfaces marked as allow-hotplug > +# and with util-linux that is as simple as adding --allow=hotplug. > +# Busybox unfortunately doesn't have this option. > +# allow-hotplug is a pattern like eth0 /eth* /eth*/1 /eth*=eth. > +# This function checks if INTERFACE matches an allow-hotplug pattern. > + > +allow_hotplug() { > + allow_hotplug="$(sed -n -e 's/^allow-hotplug \+\([^= ]*\).*/\1/p' /etc/network/interfaces)" > + for pattern in $allow_hotplug; do > + options="$(echo $pattern | sed -n -e 's,^/\?[^ /]\+/\(.*\),\1,p')" > + value="$(echo $pattern | sed -n -e 's,^/\?\([^ /]\+\).*,\1,p')" > + interfaces="$(ls -d /sys/class/net/$value 2>/dev/null | xargs -r -n 1 basename)" > + if [ "$options" != "" ]; then > + interfaces="$(echo $interfaces | awk -v n=$options '{print $n }')" > + fi > + echo "$interfaces" | grep -w -q "$INTERFACE" > + if [ $? -eq 0 ]; then > + return 0 > + fi > + done > + > + return 1 > +} > + > +if ! allow_hotplug; then > + exit 0 > +fi > + > # if this interface has an entry in /etc/network/interfaces, let ifupdown > # handle it > if grep -q "iface \+$INTERFACE" /etc/network/interfaces; then > Firstly thanks for the patch, we should try and fix issues like this. I feel I should mention our code comes from a backdrop of resource constrained devices and the above code makes me cringe a bit due to the execution overhead of it. We've purposefully kept the code called from udev and in our initscripts relatively minimal/simple as the overhead of executing multiple programs does build up over time. Taking the above, we have loops, then pipelines, each of which runs more commands. Each command has a fork/exec overhead. Is there some way we can simplify this rather than all the shell pipelines and loops? Cheers, Richard ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices 2025-04-09 8:37 ` Richard Purdie @ 2025-04-09 9:08 ` Jeroen Hofstee 2025-04-09 9:27 ` Richard Purdie 2025-04-09 9:28 ` Mike Looijmans 1 sibling, 1 reply; 11+ messages in thread From: Jeroen Hofstee @ 2025-04-09 9:08 UTC (permalink / raw) To: Richard Purdie, openembedded-core; +Cc: Jeroen Hofstee [-- Attachment #1: Type: text/plain, Size: 4029 bytes --] Hello Richard, On 4/9/25 10:37, Richard Purdie wrote: > On Mon, 2025-04-07 at 19:49 +0200, Jeroen Hofstee via lists.openembedded.org wrote: >> From: Jeroen Hofstee<jhofstee@victronenergy.com> >> >> Commit 160f713917 (udev-extraconf: fix network.sh script did not >> configure hotplugged interfaces, 2024-10-18) fixed ifupdown for >> hotplug devices, but also calls it for non hotplug devices. That >> can cause issue, since they might not expect the ifupdown from >> udev, since it wasn't called before mentioned patch got merged. >> >> For util-linux this can simply be fixed by adding --allow=hotplug. >> Unfortunately busybox doesn't have that option, so a function is >> added to check if the device is marked as allow-hotplug. >> >> Since wilcards are supported adding 'allow-hotplug *' allows to >> restore behaviour of mentioned patch, while this restores the >> original behaviour. >> >> Signed-off-by: Jeroen Hofstee<jhofstee@victronenergy.com> >> --- >> .../udev/udev-extraconf/network.sh | 28 +++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/meta/recipes-core/udev/udev-extraconf/network.sh b/meta/recipes-core/udev/udev-extraconf/network.sh >> index 500e60ae61..da79f00d5a 100644 >> --- a/meta/recipes-core/udev/udev-extraconf/network.sh >> +++ b/meta/recipes-core/udev/udev-extraconf/network.sh >> @@ -6,6 +6,34 @@ echo "$INTERFACE" | grep -q wifi && exit 0 >> # udevd does clearenv(). Export shell PATH to children. >> export PATH >> >> +# udev should only trigger ifupdown for interfaces marked as allow-hotplug >> +# and with util-linux that is as simple as adding --allow=hotplug. >> +# Busybox unfortunately doesn't have this option. >> +# allow-hotplug is a pattern like eth0 /eth* /eth*/1 /eth*=eth. >> +# This function checks if INTERFACE matches an allow-hotplug pattern. >> + >> +allow_hotplug() { >> + allow_hotplug="$(sed -n -e 's/^allow-hotplug \+\([^= ]*\).*/\1/p' /etc/network/interfaces)" >> + for pattern in $allow_hotplug; do >> + options="$(echo $pattern | sed -n -e 's,^/\?[^ /]\+/\(.*\),\1,p')" >> + value="$(echo $pattern | sed -n -e 's,^/\?\([^ /]\+\).*,\1,p')" >> + interfaces="$(ls -d /sys/class/net/$value 2>/dev/null | xargs -r -n 1 basename)" >> + if [ "$options" != "" ]; then >> + interfaces="$(echo $interfaces | awk -v n=$options '{print $n }')" >> + fi >> + echo "$interfaces" | grep -w -q "$INTERFACE" >> + if [ $? -eq 0 ]; then >> + return 0 >> + fi >> + done >> + >> + return 1 >> +} >> + >> +if ! allow_hotplug; then >> + exit 0 >> +fi >> + >> # if this interface has an entry in /etc/network/interfaces, let ifupdown >> # handle it >> if grep -q "iface \+$INTERFACE" /etc/network/interfaces; then >> > Firstly thanks for the patch, we should try and fix issues like this. I > feel I should mention our code comes from a backdrop of resource > constrained devices and the above code makes me cringe a bit due to the > execution overhead of it. We are still supporting omap3 / 600MHz, so I understand that. Typically there isn't an allow-hotplug entry in /etc/network/interfaces, so it is just running a sed over /etc/network/interfaces which is commonly rather small. The loop is only executed if you have allow-hotplug entries. > We've purposefully kept the code called from udev and in our > initscripts relatively minimal/simple as the overhead of executing > multiple programs does build up over time. Taking the above, we have > loops, then pipelines, each of which runs more commands. Each command > has a fork/exec overhead. > > Is there some way we can simplify this rather than all the shell > pipelines and loops? The simplest solution is to patch busybox to respect --allow=hotplug I guess. With kind regards, Jeroen [-- Attachment #2: Type: text/html, Size: 5064 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices 2025-04-09 9:08 ` Jeroen Hofstee @ 2025-04-09 9:27 ` Richard Purdie 2025-04-09 10:40 ` Jeroen Hofstee 0 siblings, 1 reply; 11+ messages in thread From: Richard Purdie @ 2025-04-09 9:27 UTC (permalink / raw) To: Jeroen Hofstee, openembedded-core; +Cc: Jeroen Hofstee On Wed, 2025-04-09 at 11:08 +0200, Jeroen Hofstee wrote: > On 4/9/25 10:37, Richard Purdie wrote: > > On Mon, 2025-04-07 at 19:49 +0200, Jeroen Hofstee via lists.openembedded.org wrote: > > From: Jeroen Hofstee <jhofstee@victronenergy.com> > > > > > > Commit 160f713917 (udev-extraconf: fix network.sh script did not > > > configure hotplugged interfaces, 2024-10-18) fixed ifupdown for > > > hotplug devices, but also calls it for non hotplug devices. That > > > can cause issue, since they might not expect the ifupdown from > > > udev, since it wasn't called before mentioned patch got merged. > > > > > > For util-linux this can simply be fixed by adding --allow=hotplug. > > > Unfortunately busybox doesn't have that option, so a function is > > > added to check if the device is marked as allow-hotplug. > > > > > > Since wilcards are supported adding 'allow-hotplug *' allows to > > > restore behaviour of mentioned patch, while this restores the > > > original behaviour. > > > > > > Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com> > > > --- > > > .../udev/udev-extraconf/network.sh | 28 +++++++++++++++++++ > > > 1 file changed, 28 insertions(+) > > > > > > diff --git a/meta/recipes-core/udev/udev-extraconf/network.sh b/meta/recipes-core/udev/udev-extraconf/network.sh > > > index 500e60ae61..da79f00d5a 100644 > > > --- a/meta/recipes-core/udev/udev-extraconf/network.sh > > > +++ b/meta/recipes-core/udev/udev-extraconf/network.sh > > > @@ -6,6 +6,34 @@ echo "$INTERFACE" | grep -q wifi && exit 0 > > > # udevd does clearenv(). Export shell PATH to children. > > > export PATH > > > > > > +# udev should only trigger ifupdown for interfaces marked as allow-hotplug > > > +# and with util-linux that is as simple as adding --allow=hotplug. > > > +# Busybox unfortunately doesn't have this option. > > > +# allow-hotplug is a pattern like eth0 /eth* /eth*/1 /eth*=eth. > > > +# This function checks if INTERFACE matches an allow-hotplug pattern. > > > + > > > +allow_hotplug() { > > > + allow_hotplug="$(sed -n -e 's/^allow-hotplug \+\([^= ]*\).*/\1/p' /etc/network/interfaces)" > > > + for pattern in $allow_hotplug; do > > > + options="$(echo $pattern | sed -n -e 's,^/\?[^ /]\+/\(.*\),\1,p')" > > > + value="$(echo $pattern | sed -n -e 's,^/\?\([^ /]\+\).*,\1,p')" > > > + interfaces="$(ls -d /sys/class/net/$value 2>/dev/null | xargs -r -n 1 basename)" > > > + if [ "$options" != "" ]; then > > > + interfaces="$(echo $interfaces | awk -v n=$options '{print $n }')" > > > + fi > > > + echo "$interfaces" | grep -w -q "$INTERFACE" > > > + if [ $? -eq 0 ]; then > > > + return 0 > > > + fi > > > + done > > > + > > > + return 1 > > > +} > > > + > > > +if ! allow_hotplug; then > > > + exit 0 > > > +fi > > > + > > > # if this interface has an entry in /etc/network/interfaces, let ifupdown > > > # handle it > > > if grep -q "iface \+$INTERFACE" /etc/network/interfaces; then > > > > Firstly thanks for the patch, we should try and fix issues like this. I > > feel I should mention our code comes from a backdrop of resource > > constrained devices and the above code makes me cringe a bit due to the > > execution overhead of it. > > We are still supporting omap3 / 600MHz, so I understand that. Typically > there isn't an allow-hotplug entry in /etc/network/interfaces, so it is just > running a sed over /etc/network/interfaces which is commonly rather > small. > > The loop is only executed if you have allow-hotplug entries. This code dates from the omap3 era! You're right, the common case is probably just the sed which isn't so bad. Seeing blocks of complex code like that is going to make it harder for me to argue against keeping things simple in future though which makes me torn on this patch. > > We've purposefully kept the code called from udev and in our > > initscripts relatively minimal/simple as the overhead of executing > > multiple programs does build up over time. Taking the above, we have > > loops, then pipelines, each of which runs more commands. Each command > > has a fork/exec overhead. > > > > Is there some way we can simplify this rather than all the shell > > pipelines and loops? > > The simplest solution is to patch busybox to respect --allow=hotplug I guess. Yes, I as wondering about that. I see patches from a long time ago but I guess they were never merged. It probably is easier for busybox to handle this rather than shell code but I don't know what chance they'd have of being merged upstream. I would probably perfer to fix busybox to support this but I don't know how well that is going to work out... Cheers, Richard ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices 2025-04-09 9:27 ` Richard Purdie @ 2025-04-09 10:40 ` Jeroen Hofstee 2025-04-09 10:52 ` Richard Purdie 2025-04-10 10:14 ` Alexander Kanavin 0 siblings, 2 replies; 11+ messages in thread From: Jeroen Hofstee @ 2025-04-09 10:40 UTC (permalink / raw) To: richard.purdie, Jeroen Hofstee, openembedded-core; +Cc: Jeroen Hofstee Hello Richard, On 4/9/25 11:27, Richard Purdie via lists.openembedded.org wrote: >>> We've purposefully kept the code called from udev and in our >>> initscripts relatively minimal/simple as the overhead of executing >>> multiple programs does build up over time. Taking the above, we have >>> loops, then pipelines, each of which runs more commands. Each command >>> has a fork/exec overhead. >>> >>> Is there some way we can simplify this rather than all the shell >>> pipelines and loops? >> The simplest solution is to patch busybox to respect --allow=hotplug I guess. > Yes, I as wondering about that. I see patches from a long time ago but > I guess they were never merged. It probably is easier for busybox to > handle this rather than shell code but I don't know what chance they'd > have of being merged upstream. > > I would probably perfer to fix busybox to support this but I don't know > how well that is going to work out... Unless they changed their mind, upstream busybox won't accept it, so it has to be a patch in OE. But feel free to try to upstream it. For completeness, this is not some theoretical issue, always calling if up directly after a network device is discovered does cause issues and is against spec as well. The sed stuff isn't that complicated, is it? Regards, Jeroen ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices 2025-04-09 10:40 ` Jeroen Hofstee @ 2025-04-09 10:52 ` Richard Purdie 2025-04-10 10:14 ` Alexander Kanavin 1 sibling, 0 replies; 11+ messages in thread From: Richard Purdie @ 2025-04-09 10:52 UTC (permalink / raw) To: Jeroen Hofstee, Jeroen Hofstee, openembedded-core; +Cc: Jeroen Hofstee On Wed, 2025-04-09 at 12:40 +0200, Jeroen Hofstee wrote: > Hello Richard, > > On 4/9/25 11:27, Richard Purdie via lists.openembedded.org wrote: > > > > We've purposefully kept the code called from udev and in our > > > > initscripts relatively minimal/simple as the overhead of executing > > > > multiple programs does build up over time. Taking the above, we have > > > > loops, then pipelines, each of which runs more commands. Each command > > > > has a fork/exec overhead. > > > > > > > > Is there some way we can simplify this rather than all the shell > > > > pipelines and loops? > > > The simplest solution is to patch busybox to respect --allow=hotplug I guess. > > Yes, I as wondering about that. I see patches from a long time ago but > > I guess they were never merged. It probably is easier for busybox to > > handle this rather than shell code but I don't know what chance they'd > > have of being merged upstream. > > > > I would probably perfer to fix busybox to support this but I don't know > > how well that is going to work out... > > Unless they changed their mind, upstream busybox won't accept it, so it > has to be a patch in OE. But feel free to try to upstream it. It would be worth a try at least asking as that was a long time ago and we do have a specific need here which doesn't involve feature creep (as far as I know). > For completeness, this is not some theoretical issue, always calling if up > directly after a network device is discovered does cause issues and is > against spec as well. Right, I don't doubt the issue and agree we should fix it, it is just a question of how. > The sed stuff isn't that complicated, is it? It is about the principle and precedent. If I take this, it becomes so much harder to me to argue against the next similar change and hard to keep things simple overall. I'm therefore really torn. I'm spelling things out to see if anyone else has opinions too. Mike's alternative way of handling things is potentially interesting but I worry that may also be "against spec". Cheers, Richard ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices 2025-04-09 10:40 ` Jeroen Hofstee 2025-04-09 10:52 ` Richard Purdie @ 2025-04-10 10:14 ` Alexander Kanavin 2025-04-10 12:17 ` Yoann Congal 1 sibling, 1 reply; 11+ messages in thread From: Alexander Kanavin @ 2025-04-10 10:14 UTC (permalink / raw) To: oe; +Cc: richard.purdie, Jeroen Hofstee, openembedded-core, Jeroen Hofstee On Wed, 9 Apr 2025 at 12:40, Jeroen Hofstee via lists.openembedded.org <oe=myspectrum.nl@lists.openembedded.org> wrote: > For completeness, this is not some theoretical issue, always calling if up > directly after a network device is discovered does cause issues and is > against spec as well. > > The sed stuff isn't that complicated, is it? I beg to differ. The code is basically unreadable (unless you want to write an elaborate comment with example input->output transformations for every single line it), and that in itself is reason not to take it. Any software solution must not only solve the problem, it must also be written such that other humans can understand and change it. I think I'd prefer a patch to busybox; even if upstream rejects or ignores it, we still have something that is easier to take care of. Alex ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices 2025-04-10 10:14 ` Alexander Kanavin @ 2025-04-10 12:17 ` Yoann Congal 2025-04-10 18:32 ` Jeroen Hofstee 0 siblings, 1 reply; 11+ messages in thread From: Yoann Congal @ 2025-04-10 12:17 UTC (permalink / raw) To: alex.kanavin Cc: oe, richard.purdie, Jeroen Hofstee, openembedded-core, Jeroen Hofstee [-- Attachment #1: Type: text/plain, Size: 1627 bytes --] Le jeu. 10 avr. 2025 à 12:14, Alexander Kanavin via lists.openembedded.org <alex.kanavin=gmail.com@lists.openembedded.org> a écrit : > On Wed, 9 Apr 2025 at 12:40, Jeroen Hofstee via lists.openembedded.org > <oe=myspectrum.nl@lists.openembedded.org> wrote: > > For completeness, this is not some theoretical issue, always calling if > up > > directly after a network device is discovered does cause issues and is > > against spec as well. > > > > The sed stuff isn't that complicated, is it? > > I beg to differ. The code is basically unreadable (unless you want to > write an elaborate comment with example input->output transformations > for every single line it), and that in itself is reason not to take > it. Any software solution must not only solve the problem, it must > also be written such that other humans can understand and change it. > > I think I'd prefer a patch to busybox; even if upstream rejects or > ignores it, we still have something that is easier to take care of. > FWIW, I agree with Alexander here. A patch (even a local one) will be easier to maintain in the long run. > > Alex > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#214638): > https://lists.openembedded.org/g/openembedded-core/message/214638 > Mute This Topic: https://lists.openembedded.org/mt/112138312/4316185 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [ > yoann.congal@smile.fr] > -=-=-=-=-=-=-=-=-=-=-=- > > -- Yoann Congal Smile ECS [-- Attachment #2: Type: text/html, Size: 3066 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices 2025-04-10 12:17 ` Yoann Congal @ 2025-04-10 18:32 ` Jeroen Hofstee 2025-04-10 19:05 ` Alexander Kanavin 0 siblings, 1 reply; 11+ messages in thread From: Jeroen Hofstee @ 2025-04-10 18:32 UTC (permalink / raw) To: yoann.congal, alex.kanavin Cc: richard.purdie, Jeroen Hofstee, openembedded-core, Jeroen Hofstee [-- Attachment #1: Type: text/plain, Size: 1444 bytes --] Hello Alex and Yoann, On 4/10/25 14:17, Yoann Congal via lists.openembedded.org wrote: > Le jeu. 10 avr. 2025 à 12:14, Alexander Kanavin via > lists.openembedded.org <http://lists.openembedded.org> > <alex.kanavin=gmail.com@lists.openembedded.org> a écrit : > > On Wed, 9 Apr 2025 at 12:40, Jeroen Hofstee via > lists.openembedded.org <http://lists.openembedded.org> > <oe=myspectrum.nl@lists.openembedded.org> wrote: > > For completeness, this is not some theoretical issue, always > calling if up > > directly after a network device is discovered does cause issues > and is > > against spec as well. > > > > The sed stuff isn't that complicated, is it? > > I beg to differ. The code is basically unreadable (unless you want to > write an elaborate comment with example input->output transformations > for every single line it), and that in itself is reason not to take > it. Any software solution must not only solve the problem, it must > also be written such that other humans can understand and change it. > > I think I'd prefer a patch to busybox; even if upstream rejects or > ignores it, we still have something that is easier to take care of. > > > FWIW, I agree with Alexander here. > A patch (even a local one) will be easier to maintain in the long run. > Cool, let me know when these patches have landed. Regards, Jeroen [-- Attachment #2: Type: text/html, Size: 3153 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices 2025-04-10 18:32 ` Jeroen Hofstee @ 2025-04-10 19:05 ` Alexander Kanavin 0 siblings, 0 replies; 11+ messages in thread From: Alexander Kanavin @ 2025-04-10 19:05 UTC (permalink / raw) To: Jeroen Hofstee Cc: yoann.congal, richard.purdie, Jeroen Hofstee, openembedded-core, Jeroen Hofstee On Thu, 10 Apr 2025 at 20:33, Jeroen Hofstee <oe@myspectrum.nl> wrote: > Cool, let me know when these patches have landed. This probably wasn't clear enough: the expectation is that you write and submit any needed changes to busybox upstream, and once submitted, propose a patch here that adds them to busybox recipe. Otherwise it is not going to happen. Alex ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices 2025-04-09 8:37 ` Richard Purdie 2025-04-09 9:08 ` Jeroen Hofstee @ 2025-04-09 9:28 ` Mike Looijmans 1 sibling, 0 replies; 11+ messages in thread From: Mike Looijmans @ 2025-04-09 9:28 UTC (permalink / raw) To: openembedded-core On 09-04-2025 10:37, Richard Purdie via lists.openembedded.org wrote: > On Mon, 2025-04-07 at 19:49 +0200, Jeroen Hofstee via lists.openembedded.org wrote: >> From: Jeroen Hofstee <jhofstee@victronenergy.com> >> >> Commit 160f713917 (udev-extraconf: fix network.sh script did not >> configure hotplugged interfaces, 2024-10-18) fixed ifupdown for >> hotplug devices, but also calls it for non hotplug devices. That >> can cause issue, since they might not expect the ifupdown from >> udev, since it wasn't called before mentioned patch got merged. >> >> For util-linux this can simply be fixed by adding --allow=hotplug. >> Unfortunately busybox doesn't have that option, so a function is >> added to check if the device is marked as allow-hotplug. >> >> Since wilcards are supported adding 'allow-hotplug *' allows to >> restore behaviour of mentioned patch, while this restores the >> original behaviour. >> >> Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com> >> --- >> .../udev/udev-extraconf/network.sh | 28 +++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/meta/recipes-core/udev/udev-extraconf/network.sh b/meta/recipes-core/udev/udev-extraconf/network.sh >> index 500e60ae61..da79f00d5a 100644 >> --- a/meta/recipes-core/udev/udev-extraconf/network.sh >> +++ b/meta/recipes-core/udev/udev-extraconf/network.sh >> @@ -6,6 +6,34 @@ echo "$INTERFACE" | grep -q wifi && exit 0 >> # udevd does clearenv(). Export shell PATH to children. >> export PATH >> >> +# udev should only trigger ifupdown for interfaces marked as allow-hotplug >> +# and with util-linux that is as simple as adding --allow=hotplug. >> +# Busybox unfortunately doesn't have this option. >> +# allow-hotplug is a pattern like eth0 /eth* /eth*/1 /eth*=eth. >> +# This function checks if INTERFACE matches an allow-hotplug pattern. >> + >> +allow_hotplug() { >> + allow_hotplug="$(sed -n -e 's/^allow-hotplug \+\([^= ]*\).*/\1/p' /etc/network/interfaces)" >> + for pattern in $allow_hotplug; do >> + options="$(echo $pattern | sed -n -e 's,^/\?[^ /]\+/\(.*\),\1,p')" >> + value="$(echo $pattern | sed -n -e 's,^/\?\([^ /]\+\).*,\1,p')" >> + interfaces="$(ls -d /sys/class/net/$value 2>/dev/null | xargs -r -n 1 basename)" >> + if [ "$options" != "" ]; then >> + interfaces="$(echo $interfaces | awk -v n=$options '{print $n }')" >> + fi >> + echo "$interfaces" | grep -w -q "$INTERFACE" >> + if [ $? -eq 0 ]; then >> + return 0 >> + fi >> + done >> + >> + return 1 >> +} >> + >> +if ! allow_hotplug; then >> + exit 0 >> +fi >> + >> # if this interface has an entry in /etc/network/interfaces, let ifupdown >> # handle it >> if grep -q "iface \+$INTERFACE" /etc/network/interfaces; then >> > Firstly thanks for the patch, we should try and fix issues like this. I > feel I should mention our code comes from a backdrop of resource > constrained devices and the above code makes me cringe a bit due to the > execution overhead of it. That was my first reaction as well. Immediately followed by "luckily I use mdev on constrained devices". > We've purposefully kept the code called from udev and in our > initscripts relatively minimal/simple as the overhead of executing > multiple programs does build up over time. Taking the above, we have > loops, then pipelines, each of which runs more commands. Each command > has a fork/exec overhead. > > Is there some way we can simplify this rather than all the shell > pipelines and loops? I've always found that the way that initscripts starts the network interfaces is broken. It immediately tries to bring up an interface when it detects it. It will send out DHCP requests, of which the first few usually get dropped because the interface doesn't have a carrier yet. Causing a delay of sometimes 10 seconds before the interface actually configures. So what I have been putting in there for years is to start "ifplugd" for interfaces when they arrive (cold and hotplug treated the same). And once ifplugd reports a link "up", I call "ifup" on that interface. Then the DHCP actually works immediately and the configuration finishes quickly. That also works great for wifi devices, because a "link up" means "associated with an AP", so all it takes for a wifi link is to start wpa_supplicant when it registers. This works for mdev, and should work for udev as well (after adding device add/remove hooks). Recipes are on github (ifplugd-auto-net): https://github.com/topic-embedded-products/topic-platform/tree/scarthgap/meta-topic-platform/recipes-topic/network Maybe it'd be interesting to move these to core? Mike. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-04-10 19:06 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-04-07 17:49 [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices Jeroen Hofstee 2025-04-09 8:37 ` Richard Purdie 2025-04-09 9:08 ` Jeroen Hofstee 2025-04-09 9:27 ` Richard Purdie 2025-04-09 10:40 ` Jeroen Hofstee 2025-04-09 10:52 ` Richard Purdie 2025-04-10 10:14 ` Alexander Kanavin 2025-04-10 12:17 ` Yoann Congal 2025-04-10 18:32 ` Jeroen Hofstee 2025-04-10 19:05 ` Alexander Kanavin 2025-04-09 9:28 ` Mike Looijmans
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox