Fix global variables

[FransUrbo]

Motivation and Context

A previous commit (e865e78), the local keyword was removed because of bashism.
Removing bashisms are correct, however this caused multiple functions using the same variable to overwrite each other.
So this commit make function variables unique in the (now) global namespace.

The intention was to only allow such file systems that are under the root of the root file system to mount - a nested file system hierarchy. This was so that multiple operating systems should be allowed to co-exist in the same pool.

However, this was broken and caused ALL file systems in the pool with the canmount=noauto to be mounted, because the ${fs} variable was used in many places, and got overwritten.

Second commit is to have good shell scripting practices - variable names should be wrapped in {} and be enclosed by ".

Fixes: #17963

Description

Prevent the previously local variables, now global, from being overwritten by another function by renaming them to something unique.

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[FransUrbo]

I'm perfectly happy to drop this commit.

I can't remember where I saw it, or when, but it's more than ten years ago, that "good shell script practices" was to write variables as ${variable}, not $variable.

Weather that's true or not, it IS annoying to me to see both side by side - PICK ONE!! :D :D.

[robn]

Good grief, what a mess. Good work tracking all this down.

One low-key suggestion: I wonder if it would be useful to prefix the "local" variables with an underscore, just to make them easier to see?

I have not tested and don't have this problem, but based on the description and changes I can't obviously see how it could do anything worse, assuming the original logic was fine. Approving on that basis.

Thanks!

[behlendorf]

Ugh, this script clearly needed some attention. Thanks for the cleanup.

I haven't manually tested this either, but reading through it makes sense and is definitely an improvement.

[FransUrbo]

Thanx guys, but for complete transparency, I haven't tested it myself!

Now, I could setup a new initrd (which I've done in the past obviously, but it was quite a few years ago :) and do the test on say a Debian GNU/Linux box, but if this is used in many other places - it have at least at some point been used by Debian/kFreeBSD (which is no more unfortunately), Ubuntu and Gentoo and others.. I can't test them all!

However, I do (vaguely!) remember that we had a talk about this being in the ZFS repo itself, instead of letting each dist/os do their own. Our compromise was contrib - "no support, but if you want to use it, here it is". ?

Not meaning we should abandon it completely, but my thinking here is that we do the best "ocular inspection" we can, and if we all think it's ok, then we merge it and we fix any problems users/dists report.. ?

I know for a fact that the ONLY bug that's been reported (or at least the only one I've seen!) about this is that Debian GNU/Linux one, and it was for the testing dist - which is known to be a bit .. "fritzzy" at times :).

[FransUrbo]

I'm perfectly happy to test this on at least one system if you want me to, I don't really have much else to do now for the next two weeks :).

One low-key suggestion: I wonder if it would be useful to prefix the "local" variables with an underscore, just to make them easier to see?

I think that would be very sensible thing to do, I'll update the commits.