Bash: Use variable in loop

Question

I'm in need of some help (again) after you guys helped me wonderfully last time. I'm writing a script to test my HDD's with SmartCTL. So I can ofcourse copy the commands per disk, but it would be even more awesome if I can just set the disks once and the script does them one by one:

#!/bin/bash
 date=`date +%d-%m-%Y-%T`
 touch /var/log/disk/Disk-health-check-$date
 disks="/dev/sda 
 /dev/sdb"
 for disk in disks
    do
    wait=$(smartctl -t short $disk | awk '/Please wait/ {print $3}')
      echo "waiting..."
      sleep $((wait * 60 + 60))
      echo "done"
      smartctl --log=selftest $disk
      smartctl -a $disk
   done
 exit

But unfortunately it only uses the /dev/sda, and not sdb, etc. So how can I make this works? Thanks in advance guys!!

for disk in $disks, not for disk in disks. Though even that is bad practice. — Charles Duffy
– Charles Duffy, Commented Jan 30, 2017 at 17:49
disks=( /dev/sda /dev/sdb ), and use for disk in "${disks[@]}", for an array. And fix all the bugs shellcheck.net finds. — Charles Duffy
– Charles Duffy, Commented Jan 30, 2017 at 17:50
as another aside -- use YYYY-mm-dd as your date format. It's an ISO standard, and -- more importantly -- its ASCII sort order lines up with its sort order as a date, which is absolutely not true for the format you're trying to use here. Having ASCII sort order be correct makes it way easier to find the oldest or newest file, or every file older than a specific date, etc. — Charles Duffy
– Charles Duffy, Commented Jan 30, 2017 at 17:52

Charles Duffy · Accepted Answer · 2017-01-30 20:33:41Z

4

Try this:

for disk in disks; do
  echo "$disk"
done

You'll see that the only thing it echos is disks. That's because you're telling it to iterate over exactly one value, and that value is disks.

To do the subtly buggy thing you were trying to do, you want instead:

for disk in $disks; do
  echo "$disk"
done

However, as I said, that's buggy. Let's say your disks variable were assigned a bit differently:

disks='
/dev/disks/by-label/My Drive
/dev/disks/by-label/Other Drive
/dev/disks/by-label/* TEENAGE DAUGHTER'S DRIVE *
'

This would have /dev/disks/by-label/My as one entry, Drive as the next -- and the *s would be expanded to the names of files in the directory you're in when you run the script. Obviously not what you want.

Instead, use an array:

disks=(
  "/dev/disks/by-label/My Drive"
  "/dev/disks/by-label/Other Drive"
  "/dev/disks/by-label/* TEENAGE DAUGHTER'S DRIVE *"
)

...and iterate over them as:

for disk in "${disks[@]}"; do
  echo "Processing: $disk"
done

edited Jan 30, 2017 at 20:33

answered Jan 30, 2017 at 17:58

Charles Duffy

299k43 gold badges441 silver badges497 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Florius Over a year ago

Thank you, that's pretty sweet. Would you mind explaining "${disks[@]}" how that actually works? I've never seen that before to be honest. Is there any difference using ("disks disk") or disks=(disk1 disk2)? (Yours and Chuck Donahue's)?

Charles Duffy Over a year ago

My answer and Chuck's are effectively equivalent, since the content at hand doesn't need to be quoted. disks=( "/dev/sda" "/dev/sdb" ) would work too, in the case where the names don't contain any spaces or glob characters. I'm showing the quoting above to demonstrate how it works when needed with an array, whereas it wouldn't work with disks='"/dev/sda" "/dev/sdb"' or such; see BashFAQ #50 for a detailed discussion of the failure mode.

Charles Duffy Over a year ago

As for "${disks[@]}", that's standard array expansion syntax -- see BashFAQ #5, or the bash-hackers wiki page on arrays linked from Chuck's answer.

Chuck Donahue · Accepted Answer · 2017-01-30 18:10:17Z

1

Try this:

#!/bin/bash
date=$(date +%d-%m-%Y-%T)
touch /var/log/disk/Disk-health-check-"$date"
disks=(/dev/sda /dev/sdb)
for disk in "${disks[@]}"
   do
   wait=$(smartctl -t short "$disk" | awk '/Please wait/ {print $3}')
     echo "waiting..."
     sleep $((wait * 60 + 60))
     echo "done"
     smartctl --log=selftest "$disk"
     smartctl -a "$disk"
  done
exit

Based upon Array Variables outlined here: http://wiki.bash-hackers.org/syntax/arrays?s[]=arrays

edited Jan 30, 2017 at 18:10

answered Jan 30, 2017 at 17:58

Chuck Donahue

113 bronze badges

3 Comments

Charles Duffy Over a year ago

Much better. I'd suggest directing logs to stderr rather than stdout -- since they're status (intended for user consumption) as opposed to output (to be sent to the next program in a pipeline) -- but nothing here is wrong on its face.

Chuck Donahue Over a year ago

Thanks for the link to spellcheck.net

Charles Duffy Over a year ago

err, it's "shellcheck", with an h rather than a p :)

Collectives™ on Stack Overflow

Bash: Use variable in loop

2 Answers 2

3 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related