Bug 173574

Summary: Poor coding practices
Product: [Fedora] Fedora Reporter: JW <ohtmvyyn>
Component: initscriptsAssignee: Bill Nottingham <notting>
Status: CLOSED RAWHIDE QA Contact: Brock Organ <borgan>
Severity: medium Docs Contact:
Priority: medium    
Version: 4CC: 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 Flags
Minor init.d/network and ifup-eth cleanups none

Description JW 2005-11-18 08:45:04 UTC
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b) Gecko/20050217

Description of problem:
Many of the initscripts exhibit bad scripting habits. There are numerous instances where instead of sourcing scripts there are awful fgreps and greps to extract needed definitions.
For example, init.d/network would have to be one of the worst.

This piece of code:

    for i in $interfaces; do
        eval $(LANG=C fgrep ^DEVICE= ifcfg-$i)

would have to be one of the most grotesque fragments of shell scripting that I have ever seen.  It is wrong for the following reasons:

1. If an interface has no DEVICE definition then it will inherit the definition from previous loop definition
2. Whitespace before DEVICE will fail
3. If the ifcfg-$i sources another file then the DEVICE defintion wont be found
4. It is plain ridiculous

What is wrong with this:

    for i in $interfaces; do
    (
        . ifcfg-$i
        <now play with $DEVICE>
    )
    done

I cannot undertsand why people have to grep bash script!  Surely bash can do a better job parsing it!



Version-Release number of selected component (if applicable):
All versions

How reproducible:
Always

Steps to Reproduce:
1. read
2. the 
3. lame
4. scripts
  

Additional info:

Comment 1 Bill Nottingham 2005-11-18 17:13:10 UTC
Well, 1. and 3. are invalid anyway. But it can be fixed.

Comment 2 Miloslav Trmač 2006-03-05 15:31:08 UTC
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.

Comment 3 Miloslav Trmač 2006-03-05 21:11:29 UTC
Created attachment 125677 [details]
Minor init.d/network and ifup-eth cleanups

- don't duplicate unset
- remove two (eval `grep ...`) instances

Comment 4 JW 2006-03-05 22:42:27 UTC
(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.


Comment 5 Miloslav Trmač 2006-03-05 22:58:17 UTC
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.

Comment 6 JW 2006-03-05 23:17:19 UTC
(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).


Comment 7 Miloslav Trmač 2006-03-05 23:46:17 UTC
(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.

Comment 8 JW 2006-03-05 23:56:41 UTC
(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!


Comment 9 Miloslav Trmač 2006-03-06 00:10:26 UTC
(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).

Comment 10 JW 2006-03-06 00:19:00 UTC
> 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.


Comment 11 Denis Ovsienko 2006-03-11 06:19:58 UTC
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.

Comment 12 JW 2006-03-11 06:28:00 UTC
(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, ...


Comment 13 Miloslav Trmač 2006-04-10 22:59:08 UTC
The cleanups from commment #3 were applied in initscripts-8.32-1.  Thanks
for your report.