review my shell script
1 Attachment(s)
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. |
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! |
Since shell scripts are plain text, you can easily attach yours to a post. Just add a .txt extension to it.
|
the very first thing you can do is to run shellcheck
|
@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? |
@hazel thanks for the suggestion, I added a .txt file to the 1st post.
|
@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.
|
Forgot to mention that I edited the 1st post to explain what the code does, as suggested by @astrogeek.
|
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)) Here's how you lower a string in bash (without using awk) Code:
family_arg="$(printf '%s' "$family_arg" | awk '{print tolower($0)}')" 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 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 Code:
# don't print any lines with "dev lo" |
Code:
set -- "$args" The following is slightly better: Code:
set -f Code:
## Simple args parsing, options must be first |
Code:
command -v ... I would set a long generic PATH at the beginning of the script Code:
PATH=$( A text line should end with a newline; use one of the following: Code:
printf "%s\n" "32" | grep ... |
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)}')" |
Quote:
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 |
Quote:
A lot of programs write error messages to stderr, suppressing them may cause headache.... Debug messages should be handled independently |
Quote:
Thanks for the heads-up. |
All times are GMT -5. The time now is 05:40 PM. |