Bug 173574
Summary: | Poor coding practices | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | JW <ohtmvyyn> | ||||
Component: | initscripts | Assignee: | Bill Nottingham <notting> | ||||
Status: | CLOSED RAWHIDE | QA Contact: | Brock Organ <borgan> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | 4 | CC: | denis, mitr, rvokal | ||||
Target Milestone: | --- | ||||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | 8.32-1 | Doc Type: | Bug Fix | ||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2006-04-10 22:59:08 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 150221 | ||||||
Attachments: |
|
Description
JW
2005-11-18 08:45:04 UTC
Well, 1. and 3. are invalid anyway. But it can be fixed. cipeinterfaces="" for i in $interfaces; do ( . ifcfg-$i if ...; then cipeinterfaces="$cipeinterfaces $i" ) done ... use $cipeinterfaces won't work. There are a few ways to make the script prettier, but using grep is one of the better solutions in this particular case. Created attachment 125677 [details]
Minor init.d/network and ifup-eth cleanups
- don't duplicate unset
- remove two (eval `grep ...`) instances
(In reply to comment #2) > cipeinterfaces="" > for i in $interfaces; do > ( > . ifcfg-$i > if ...; then > cipeinterfaces="$cipeinterfaces $i" > ) > done > ... use $cipeinterfaces > > won't work. There are a few ways to make the script prettier, but using > grep is one of the better solutions in this particular case. Of course it wont work, you just have to use your imagination! Like this: for i in $interfaces; do ( . ifcfg-$i if ...; then exit 0 else exit 1 ) && cipeinterfaces="$cipeinterfaces $i" Come on Redhat! You are supposed to be experts!! You should know how to handle subshells for goodness sake. Grep is not the right way to transpose definitions in one shell script to another. The loop is actually collecting the interfaces to four different variables, so this won't work either. It would be possible to encode the subshell results in the exit code or using set `conditions here`, but that would just obfuscate the code. Please note that ifcfg-* are _not_ arbitrary shell scripts, they are parsed by several other tools (e.g. ppp-watch, s-c-network) and thus their syntax is much stricter. There is therefore no correctness gain in not using grep, there is only a minor performance gain. (In reply to comment #5) > The loop is actually collecting the interfaces to four different variables, > so this won't work either. That is strange because I have already modified my 'network' scruipt and have been using it successfully for a couple of years now. > It would be possible to encode the subshell > results in the exit code or using set `conditions here`, but that would just > obfuscate the code. Perhaps you don't understand the possible reasons to source shell scripts. For example, my ADSL modem interface has two components - the raw ifcfg-eth1 and the proper ifcfg-MYISP. The ifcfg-MYISP has to pick up some data from the ifcfg-eth1 so it sources that script. It also does plenty of other shell things - like it has if-else-fi clauses. Now if you have a shell script that you want to extract some data from, and it contains conditional statements, then your grep'ing is really going to get badly broken. > > Please note that ifcfg-* are _not_ arbitrary shell scripts, they are parsed by > several other tools (e.g. ppp-watch, s-c-network) and thus their syntax is much > stricter. There is therefore no correctness gain in not using grep, there > is only a minor performance gain. There is a gain in not using grep - things work more better (sorry about the MSism). (In reply to comment #6) > Perhaps you don't understand the possible reasons to source shell scripts. For > example, my ADSL modem interface has two components - the raw ifcfg-eth1 and > the proper ifcfg-MYISP. The ifcfg-MYISP has to pick up some data from the > ifcfg-eth1 so it sources that script. It also does plenty of other shell > things - like it has if-else-fi clauses. You have quoted it, but let me point it out again: > Please note that ifcfg-* are _not_ arbitrary shell scripts, they are parsed by > several other tools (e.g. ppp-watch, s-c-network) and thus their syntax is > much stricter. If your ifcfg files contain anything other lines matching /^#.*$/ or /^[a-zA-Z_][a-zA-Z0-9_]*=\(".*"\"'.*'|[^'"].*\), the configuration is relying on what are AFAIK undocumented aspects of the historical implementation [1] and the configuration is unsupported also by other tools than just /etc/init.d/network. It is no more reasonable to expect tools written in C or Python to handle full shell script syntax than to expect using (eval `grep ^VARNAME= script`) to handle general shell scripts. [1] No, I can't point to an exact documentation of the file format, but at least the RHEL Reference Guide is talking about "configuration files" and not "shell scripts", and the lack of documentation prohibiting usage of shell features is not a promise that shell features will work. (In reply to comment #7) > > Please note that ifcfg-* are _not_ arbitrary shell scripts, they are parsed by > > several other tools (e.g. ppp-watch, s-c-network) and thus their syntax is > > much stricter. I do believe that the above two programs you mention do *not* reparse and of these files because they are passed appropriate arguments when they are invoked - or have something in environment. And if that wasn't the case then their design is flawed. Simple programs should have explicit parameters passed, rather than have significant parameters buried in some obscure file. In actual fact, as I have alread said, I have been running modified scripts for several years without any problem. Perhaps, in the interests of improving configurability, RH should consider decreeing that such files are to be sourceable shell-scripts. There would be considerable advantages in doing so. Not least of which is eliminating all those painful LC_ALL=C contortions which have to be peppered throught all actual shell scripts, only so that so-called no-shell-scripts can be grep'ed! (In reply to comment #8) > > > Please note that ifcfg-* are _not_ arbitrary shell scripts, they are parsed by > > > several other tools (e.g. ppp-watch, s-c-network) and thus their syntax is > > > much stricter. > > I do believe that the above two programs you mention do *not* reparse and of > these files Feel free to read the source. > And if that wasn't the case then their design is flawed. Simple programs > should have explicit parameters passed, rather than have significant > parameters buried in some obscure file. Which "parameters" would that be for system-config-network? > Perhaps, in the interests of improving configurability, RH should consider > decreeing that such files are to be sourceable shell-scripts. How would system-config-network (a GUI for editing network configuration, including the ifcfg files) work in the presence of arbitrary shell constructs? > Not least of which is eliminating all > those painful LC_ALL=C contortions which have to be peppered throught all > actual shell scripts, only so that so-called no-shell-scripts can be grep'ed! Using LC_ALL=C has nothing at all to do with greppability of ifcfg files, it is either necessary for parsing of tool output (to avoid output translated to other languages, e.g. for ifconfig), or a small performance optimization (e.g. most simple uses of grep). > How would system-config-network (a GUI for editing network configuration, > including the ifcfg files) work in the presence of arbitrary shell constructs? Perhaps s-c-network isn't a "simple" program. But it could be modified (but possibly no reconfiguration would be required) to hande ifcfg-* beging sourceable shell scripts. Feel free to try it any time you like. > Using LC_ALL=C has nothing at all to do with greppability of ifcfg files, it is > either necessary for parsing of tool output (to avoid output translated to > other languages, e.g. for ifconfig), or a small performance optimization > (e.g. most simple uses of grep). I am shocked! LC_ALL will change the collating order!! And you still have code with [a-z] etc instead of [:lower:]. Besides wouldn't it be better to invoke a 'cgrep' instead of replicating all those LC_ALL's everywhere? Anyhow I give up. I have absolutely no influence over RH employees. But if I did I would most certainly instruct them to clean up their act. Because dirty code means bugs. JW, you should take it easier. initscripts do their job, but there are some jobs they are just inappropriate for. That's why /etc/net was started. (In reply to comment #11) > JW, you should take it easier. initscripts do their job, but there are some jobs > they are just inappropriate for. That's why /etc/net was started. I beg to differ. I have managed, without too much trouble, to make them work as required without reinventing any wheel. Just a small tweek is all that was required. And as such I have been using complex ADSL configuration for a couple of years now without any bother. I just do "ifup whatever/ifdown whatever" without any fuss. The *only* requirement is to allow the ifcfg-*'s be treated as scripts. The proof is in the pudding. It works. And I cannot use /etc/net because it doesn't exist on my computer. And I like to keep things simple, efficient, concise, ... The cleanups from commment #3 were applied in initscripts-8.32-1. Thanks for your report. |