review my shell script

anton_lq · 11-16-2023, 07:37 PM

Hi all,

There is this shell code project I'm working on (as a hobby). I don't have a professional training, so I have some degree of uncertainty whether I'm breaking some widely agreed-on conventions and best practices.

If anyone cares to review a part that's more-or-less complete, I'd be interested to hear your remarks (as long as they're expressed in a productive way). This particular part is supposed to be POSIX-compliant, as much as it can be, at least.

I don't think that the code is at a complete noob level, so it's hopefully not an extremely boring thing to look at.

Edit:
Since apparently a github link is frowned upon, I attached a .txt file with the code as suggested by @hazel

Additionally, I'll explain what the code does, as suggested by @astrogeek. This is basically a script that attempts to detect local-area ip addresses of the machine it's running on, regardless of whether it's router or a host. While this seems more straightforward with ipv6, I had to use some heuristics for ipv4. I'm curious whether people here think that the heuristics are good. Then the script aggregates found ip addreses, meaning that it trims down each of them to its mask bits to get the subnet it belongs to, and then eliminates subnets that are enclosed inside other subnets in the resulting list.

The point of all that is automated firewall rules creation which I'm doing in the larger project.

BTW if someone here wants to help me test this part, I would appreciate it.

astrogeek · 11-16-2023, 09:25 PM

Welcome to LQ and the programming forum!

This is not the right place to post links to projects, whether asking for review or simply advertising. In this age of spam and exploitation it is hard to tell the difference. For that reason the link you posted has been moderated.

Additionally, you have not provided any description at all of what the purpose or intended use of your script may be so it is asking a bit much for most members to give it a look.

If you are having specific problems you need help with, or for just about any specific programming question - this is the place!

Please review the Site FAQ for guidance in asking well formed questions. Especially visit the link from that page, How to Ask Questions the Smart Way for discussion of things to consider when asking others for help.

Good luck!

hazel · 11-17-2023, 12:00 AM

Since shell scripts are plain text, you can easily attach yours to a post. Just add a .txt extension to it.

pan64 · 11-17-2023, 01:22 AM

the very first thing you can do is to run shellcheck

anton_lq · 11-17-2023, 12:09 PM

@astrogeek thanks for replying. I'm used to asking my specific questions to the search engine. The one thing that it can't do is answer the question "is there something you know that I'm missing?" which is essentially what my post boils down to. Unfortunately, looks like there is no place to ask it.
I do have a more specific code question which I'll ask in a separate topic. But for now, I want to ask another policy question - is it allowed to ask people to test my code?

anton_lq · 11-17-2023, 12:28 PM

@hazel thanks for the suggestion, I added a .txt file to the 1st post.

anton_lq · 11-17-2023, 12:30 PM

@pan64 thanks, I'm using shellcheck while working on the project, as you'll see if you check out the file I now attached to the 1st post, as it has a fair amount of 'shellcheck disable' directives. It is a valuable tool, I agree.

anton_lq · 11-17-2023, 12:51 PM

Forgot to mention that I edited the 1st post to explain what the code does, as suggested by @astrogeek.

ychaouche · 11-21-2023, 10:41 AM

Hello anton_lq,

I didn't go into the details of all the program,
but since this is a conversation forum,
I hope my few suggestions spark more improvements from the members.

1/ on parsing arguments

Here's another way to do it,
this is an excerpt from one of my scripts

Code:

while (($# > 0))
do
    # "$1" is what's being currently parsed
    case "$1" in
        # simple options that don't take an argument
	--all)
	    search_dirs="$NOTES_TXT $NOTES_LOG $NOTES_FLOWS $NOTES_HOWM $NOTES_IRC"
	    ;;
	# [...]
	# options that take an argument,
	# you simply shift the arguments
	# which makes room for the next thing being parsed as $1
	# this has the benefit of preparing for the next iteration
	# if you call your script like this :
	# foo --newer-than 30 --all
	# 
	# and the current thing being parsed is --newer-than
	# then by shifting arguments
	# the next thing that will be parsed will be --all
	# instead of the argument to --newer-than 30

	--newer-than)
	    shift
	    find_recent="-mtime -$1"
	    ;;
        #	    
        # [...]
	# also provide for the standard -- argument
	# which simply means there are no more options
	# everything that's after this are arguments
	--)
	    shift
	    pattern="$*"
	    ;;
    esac
    shift 
done

2/ lowering strings
Here's how you lower a string in bash (without using awk)

Code:

family_arg="$(printf '%s' "$family_arg" | awk '{print tolower($0)}')"

becomes

Code:

family_arg="${family_arg,,}

3/ grep, tr and sort are redundant when you already use awk

Some ideas to rewrite this line

Code:

local_ifaces_ipv4="$(ip -f inet route show table local scope link | grep -i -v ' lo ' |	awk '{for(i=1; i<=NF; i++) if($i~/^dev$/) print $(i+1)}' | sort -u)"

you are using grep + awk + sort,
you can keep only awk if you wish,
here's how:

1. instead of the for loop to look for the field,
use match and regexes.

2. instead of grep -v
use negative line matching in awk

3. instead of sort -u
use an associative array in awk
then simply print its keys.

Code:

# print all interfaces
root#ns2 17:24:48 /etc/bind # ip -f inet route show table local scope link | awk '{match($0,/dev (\w+)/,A); B[A[1]]++;} END {for (key in B) {print key}}'
eth0
lo
root#ns2 17:26:46 /etc/bind #

Code:

# don't print any lines with "dev lo"
root#ns2 17:26:46 /etc/bind # ip -f inet route show table local scope link | awk '!/dev lo/ {match($0,/dev (\w+)/,A); B[A[1]]++;} END {for (key in B) {print key}}'
eth0
root#ns2 17:27:17 /etc/bind #

I hope this gets the ball rolling.

MadeInGermany · 11-29-2023, 12:22 PM

Code:

set -- "$args"

This calms shellcheck, but only sets one $1

The following is slightly better:

Code:

set -f
set -- $args

The best method is shift, as suggested in the previous post.

Code:

## Simple args parsing, options must be first
while [ $# -gt 0 ]
do
    case $1 in
    ( -s ) subnets_only="true"; shift
    ;;
    ( -d ) debugmode="true"; shift
    ;;
    ( -f ) family_arg="$2"; shift 2 || { echo "Missing family after -f"; exit 1; }
    ;;
    ( -- ) shift; break
    ;;
    ( -* ) echo "illegal option $1 - ignored"; shift
    ;;
    ( * ) break
    esac
done

MadeInGermany · 11-29-2023, 02:29 PM

Code:

command -v ...

Does this work in Posix shells?
I would set a long generic PATH at the beginning of the script

Code:

PATH=$(
    pp="/usr/xpg4/bin:/bin:/usr/bin:/sbin:/usr/sbin:/usr/local/bin:$PATH"
    set -f; IFS=":"
    for p in $pp
    do
        [ -d "$p" ] && echo "$p"
    done | awk 'uniq[$0]++ == 0 { out=(out d $0); d=":" } END { print out }'
)
# echo "debug: $PATH"

and then simply assume that PATH finds the correct tools.

A text line should end with a newline; use one of the following:

Code:

printf "%s\n" "32" | grep ...
echo 32 | grep ...

pan64 · 11-30-2023, 02:05 AM

you can use a debug_echo function which can check the debug flag (on/off), do not need to do it everywhere.
you can use set -o errexit, set -o nounset, set -o pipefail (if you wish).
if speed is important. lines like this:

Code:

	input_subnets="$(printf "%s" "$input_subnets" | tr ' ' '\n' | sort -u | tr '\n' ' ' | awk '{print tolower($0)}')"

should be improved, a single awk is enough here. And also in a lot of cases you can make the calculations within the shell without using any external tools.

ychaouche · 12-07-2023, 02:15 AM

Quote:

Originally Posted by pan64

you can use a debug_echo function which can check the debug flag (on/off), do not need to do it everywhere.

My debug function just writes to /dev/stderr.
This way you can discard all debug messages with a simple redirect of stderr to /dev/null,
i.e w/o changing the source code or adding code to check for a -d/--debug command line option

pan64 · 12-07-2023, 02:32 AM

Quote:

Originally Posted by ychaouche

My debug function just writes to /dev/stderr.
This way you can discard all debug messages with a simple redirect of stderr to /dev/null,
i.e w/o changing the source code or adding code to check for a -d/--debug command line option

This is not a good idea
A lot of programs write error messages to stderr, suppressing them may cause headache....
Debug messages should be handled independently

ychaouche · 12-07-2023, 02:33 AM

Quote:

Originally Posted by pan64

A lot of programs write error messages to stderr, suppressing them may cause headache....
Debug messages should be handled independently

That's an excellent point.
Thanks for the heads-up.