Re: [OE-core] [PATCH] udev-extraconf: fix ifupdown for non hotplug devices

[Jeroen Hofstee <jeroen@myspectrum.nl>]

[-- 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 --]