Hacker News Reader

Comment by koolba | Hacker News Reader

(Warning pedantic script review)

Add a "set -e" to catch errors. Say if the disk can't be read or file can't be written.

Why reuse the same temp file? Make a new one with mktemp and auto clean it via an exit trap. As it's written this isn't concurrently safe.

Exiting -1 on error? Don't use negatives.

Wrap it all in a main() function and use locals instead of global vars.

View on HN

It's just a small bash script, and one that's apparently worked well for 10 years. Rewriting it to J2EE standards would just be a waste of time; the best outcome is that it still works the same, and the other outcome is that you introduced a new bug refactoring it.

> It's just a small bash script, and one that's apparently worked well for 10 years. Rewriting it to J2EE standards would just be a waste of time; the best outcome is that it still works the same, and the other outcome is that you introduced a new bug refactoring it.

I don't see how following best practices for bash scripting (or really shell scripting in general) can be compared to J2EE standards.

Seek quality in all your scripting so it becomes the norm. Otherwise you'll end up having crap like that in something mission critical.

You don't even need the 'smartStats' temporary file at all, since you can do that with just one grep:

  smartctl -a /dev/sda |
    grep '\(Reallocated_Sector_Ct\|Current_Pending_Sector\|Offline_Uncorrectable\|UDMA_CRC_Error_Count\)' > /root/stats

awk is probably better in this case, since you can save only the raw values (and other columns that you deem important):

   awk '/Reallocated_Sector_Ct|Current_Pending_Sector|Offline_Uncorrectable|UDMA_CRC_Error_Count/ {print $2,$10}'

Also https://www.shellcheck.net/

Reply on news.ycombinator.com