Skip to content

Conversation

@btkoch
Copy link

@btkoch btkoch commented Jan 14, 2024

Fix ethernet icon for systems with multiple NICs wherein cat /sys/class/net/e*/operstate outputs multiple lines. Icon now shows connected when one or more NICs are connected.

Fix ethernet icon for systems with multiple NICs wherein `cat /sys/class/net/e*/operstate ` outputs multiple lines. Icon now shows connected when one or more NICs are connected.
@mutageneral
Copy link

mutageneral commented Jan 31, 2024

This does not work if 2 NICs are "up" and grep return both, and then both "up" gets compared to a single "up" and return false as if it was down.
You can try the following for proof:

$ [ "$(echo -e "up\nup" | grep "up")" = 'up' ] && echo "🌐" || echo ""

This would be better instead:

cat /sys/class/net/e*/operstate 2>/dev/null | grep -q "up" && ethericon="🌐" || ethericon=""

If grep finds at least one instance of "up" it will return a "0" (no errors) return code and "🌐" will be put in the variable.
-q tells grep to be quiet, just like piping to /dev/null but better.
If grep does not find at least one instance of "up" it will return "1" (errors) return code and "❎" will be put in the variable.

You can try the following for proof:

$ echo -e "up\nup" | grep -q "up" && echo "🌐" || echo ""
🌐
$ echo -e "up\ndown" | grep -q "up" && echo "🌐" || echo ""
🌐
$ echo "down" | grep -q "up" && echo "🌐" || echo ""

@btkoch
Copy link
Author

btkoch commented Jan 31, 2024

Thanks for pointing that out. This is why I rarely open PRs.

@thetubster99
Copy link
Contributor

Small suggestion, you can change

cat /sys/class/net/e*/operstate 2>/dev/null | grep -q "up" /sys/class/net/e*/operstate && ethericon="🌐" || ethericon="❎"

to:

grep -q -m1 'up' /sys/class/net/e*/operstate && ethericon="🌐" || ethericon="❎"

It's slightly faster because cat is not needed anymore, and the -m1 option makes grep instantly stop after it has found its first match.

@mutageneral
Copy link

holy shit i was catting into grep xd

thetubster99 added a commit to thetubster99/cookedrice that referenced this pull request May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants