Created attachment 276649 [details, diff] Fix mount-ro and localmount initscripts use some weird logic when dealing with $IFS variable (reading list of mountpoints to omit).
I see what you did, but I'm confused at what issue you're addressing? Can you point out what was bad with the original?
As you can see branches of the if are swapped. The idea of this snippet is to save the value of $IFS before altering it and restore it later (“restore” means assign original value if it was defined and unset otherwise). However in the original code $IFS is UNSET if it was NOT defined.
I also added “local” to vars definition in the first file, and removed no-op assignment from the second one.
I made the variables local that you suggested in commit 2050e67. However, I do not follow why we need the rest of the patch. Does the code not work correctly? If so why doesn't it?
See comment #2 > As you can see branches of the if are swapped. > > The idea of this snippet is to save the value of $IFS before altering it and > restore it later (“restore” means assign original value if it was defined and > unset otherwise). However in the original code $IFS is UNSET if it was NOT > defined. Again. What we want: 1) Store $IFS value somewhere. 2) Modify $IFS as we need to process data. 3) Restore original value. How do we achieve this. Store: 1) $OIFS = $IFS 2) $SIFS = 'n' if $IFS was NOT set Restore: 1) if $SIFS == 'n' // $IFS was not set unset $IFS else $IFS = $OIFS What it does now. Store: 1) $OIFS = $IFS 2) $SIFS = 'y' if $IFS was NOT set Restore: 1) if $SIFS == 'y' // $IFS was not set $IFS = $OIFS else // $IFS WAS set unset $IFS
Created attachment 278383 [details] Old and new ways of setting/unsetting IFS to OIFS I'm having trouble getting my brain around this logic clearly so I wrote the above code to test both ways and to be honest, both the old and new way seem to be doing the same thing. Am I missing something here, or is this really just a stylistic change? Anyhow, I changed "IFS" to X to avoid colliding with the real IFS just in case.
Created attachment 278391 [details, diff] Fix saving and restoring of $IFS Oh, God! Now I get what that strange “local X=$X” stands for! I thought it's just a mistake. Not enough practice in bash programming :(. Anyway, there was no “local IFS=$IFS” in localmount and it DID NOT work. Just remove “local X=$X” from your test and you'll see it. With local variables there is no need in all those dances with SIFS and OIFS. New patch introduces a stylistic change in mount-ro, but also fixes a real bug in localmount.
(In reply to comment #7) > Created attachment 278391 [details, diff] > Fix saving and restoring of $IFS > > Oh, God! Now I get what that strange “local X=$X” stands for! I thought it's > just a mistake. Not enough practice in bash programming :(. > It didn't help that I didn't emphasize that more. Take it out and you'll see how the script reverts to the incorrect logic. Anyhow, the more I think about this the more I realize that I like your way better because style does count for something when it brings clarity. @WilliamH, let's leave this open a bit longer while I think about the cleanest way to do this. BTW, to be clear, my script uses bash-isms which we should avoid in the init scripts. Still, it does demonstrate the point.
IMO, you should consider three points: 1) Incorrect logic in localmount which effectively unset $IFS in almost all real-life cases. 2) 6 lines of code in mount-ro which _do absolutely nothing_. 3) Style and readability.
@openrc Has everyone had a chance to look at this? Looks okay to commit. I'll go ahead and do so in a few days if there are no objections.
some broken shells will barf if IFS contains whitespace (like dash) when using `local` ...
(In reply to comment #11) > some broken shells will barf if IFS contains whitespace (like dash) when using > `local` ... Apply the patch removing the local then?
(In reply to comment #12) > (In reply to comment #11) > > some broken shells will barf if IFS contains whitespace (like dash) when using > > `local` ... > > Apply the patch removing the local then? I don't think we want to get rid of "local". The patch in comment #7 seems to be more straight forward to me than what we have now; I am thinking that is what we want to apply?
(In reply to comment #11) > some broken shells will barf if IFS contains whitespace (like dash) when using > `local` ... What are those shells?
All, I just noticed that if we apply the patch from comment #7, the only reference to IFS in both scripts is the line that says: local IFS=$IFS: So that makes me wonder if we could take that patch further and just remove that line from both scripts? It isn't clear to me that IFS is still being used anywhere.
You can disregard my previous comment; I see what IFS is now thanks to Anthony. :-)
Comment on attachment 278391 [details, diff] Fix saving and restoring of $IFS (In reply to comment #14) you know in the comment you quoted, i gave you a specific example right ? the code you'll need to use is: local IFS="$IFS:"
Created attachment 279343 [details] 0001-save-and-restore-IFS-correctly.patch Mike and all, this is an updated version of the patch. Is this correct? Thanks, William
it's missing X tags referring to the bug report, but otherwise looks like OK based on what Kirill has said
Ok, cool. Anthony, are you ok with the last patch I attached? If so I'll go ahead and apply it.
(In reply to comment #20) > Ok, cool. > > Anthony, are you ok with the last patch I attached? If so I'll go ahead > and apply it. Yep.
This has been applied in git, commit 0c70328.
