Below is the bash -xv output of a section of code that i seem to have messed up.

this is the function copysys

the HOMEDIR="$HOME" for those who will ask. yes i am running this as user, and yes i have already setup visudo properly. i can make the copy manually with no issue.

Thanks in advance for the help.

It looks like the "$" in front of the >/dev/null is what's messing things up. I'm not sure what you're trying to accomplish there, maybe you meant to use a "&"?

1 members found this post helpful.

hmm my reply thanking you disappeared... You are correct i wanted the &, not the $. a 2nd pair of eyes is often a great help.

That's not the only problem with this code.

This is not an array. It's just a single scalar variable. The only reason the code is working at all is because of the unquoted $f variable later on. It allows the list (there's only one entry so the only function of the loop is to copy the first variable into the second) to be word-split by the shell.

So the cp command is actually executed as this:

You can see it in your debugging output. If any of those filenames had spaces in them (including $HOMEDIR), they'd get broken up in the wrong places, and you'd get "filename not found" errors.

The proper array-setting syntax is "array=( <wordlist> )". Be sure to put quotes around any string that has spaces or globbing patterns it too (such as your "/etc/sysconfig/network-scripts-ifcfg-eth*" here, unless you actually want it to expand into the full list of files first. That may or may not be what you want).


It might also be a good idea to declare the array local to the function, unless you need to use it elsewhere in the script. (But then again, if you did, you'd probably set it outside the function anyway, right?)


In addition to the syntax error discussed before, variable expansions should always be quoted, unless you know exactly what you are doing and have a very good reason for leaving them off. Again, as shown, this is to protect the output from word-splitting and filename expansion.


This is a vital concept in scripting, so read, and reread until you fully understand, these links:
http://mywiki.wooledge.org/Arguments
http://mywiki.wooledge.org/WordSplitting
http://mywiki.wooledge.org/Quotes


Finally, a suggestion about style. Clean, consistent formatting makes code readable and more easily debuggable. Indent all your sub-commands (meaning the function contents and the for loop in this case), and clearly separate logical sections with whitespace. Add comments anywhere the code isn't completely obvious (and remember, what seems obvious to you now will not be so a year or so down the line).

Many people also think that it's more readable to place the "do/then" keywords on the same line as the "for/while/until/if" keywords, as it more clearly separates the outside block from the inside block.

In addition, environment variables are generally upper-case, so while not critical, it's good practice to keep your own user variables in lower-case or mixed-case, to help differentiate them. And using the full bracket syntax when you don't need it does nothing but clutter up the script. I recommend leaving them off.


With all this taken into account, I think the above function should probably look more like this:

Actually, as shown before, the for loop isn't really necessary. You can copy the whole list at once. I'd use cp's -t option for maximum safety.


Actually #2, it's not usually good form to store data inside a script. It would be better to import the list values from a separate file or the command line. this would allow you to update the list as necessary without worrying about editing the script itself, and possibly breaking something in it. Or at the very least put all of your user settings at the top of the script, so you don't have to hunt through the code to change things.