DJ Lucas wrote:
> Okay, so here is a more thorough review as promised (about 20 times before).
> Again, this is based mostly on my own observations formed over the past few
> years. As we all know, ego can be a bitch, ;-) and I do feel that my own
> work was disregarded in many ways, so I'd like to see input from everyone
> who participated in the the original threads to curb my own prejudice if
> it has managed to leak into this message beyond this opening statement. I
> don't find it at all impossible that my focus on the standard could be
> mis-aligned with the goals of LFS, but I don't currently think so, and
> remember that I was always strict on FHS as well until udev made that a
> complete impossibility. Anyway, onto the meat and potatoes...
> 
> The new scripts are certainly a lot easier on the eyes, unfortunately, that
> was brought on at the expense of functionality. A good portion of these
> issues were covered in the original LSB scripts prior to Bruce's rewrite.

Thanks for the feedback.  I'm sure I made a major mistake in starting 
with the original scripts and using the LSB scripts as a reference 
instead of the other way around.  I didn't purposely disregard your 
prior efforts.  I really appreciate this review.

> First, the LSB headers are not consistent with the directory naming. Two
> options here: Get rid of 'sysinit' and use the shorter 'S':
> sed 's/sysinit/S/g' -i Makefile
> sed 's/rcsysinit/rsS/' -i lfs/init.d/rc

I've changed the Makefile and lfs/init.d/rc in my sandbox ready for the 
next commit.

> Also, need to fix inittab in the book: 's/sysinit$/S/' 

I did this too.

> or option 2 is to fix
> all of the LSB headers with 's/\tS/sysinit/' in every script.
> My only argument one way or the other is to use 'S' because it is 
> aesthetically
> pleasing when compared to 'sysinit' and the format of the other directory
> names.

Agree.

> LSB defined functions belong in /lib/lsb/init_functions so that compliant
> bootscripts work as expected. Do not include the LSB definitions directly in
> the distro's functions file. Either source them in the distribution's
> functions or move the distribution's functions to the end of
> /lib/lsb/init_functions and source only the one file (preferred method as
> discussed previously in the LSB thread).

I've got the network service scripts in /lib/boot.  I don't have a 
problem with renaming /lib/boot to /lib/lsb/ and functions to 
init_functions, but I'm not sure about having two new directories in 
/lib.  I lean toward moving the LFS functions to the end of the a single 
init_functions file.

> There is no reason to redefine 35+ values every time a new script is 
> run. Once per runlevel change in rc is sufficient. One conditional
> around the source line in each script, on a known constant should be
> good, and will be more efficient, and nominally faster than sourcing
> 2 or 3 files for every script that is run.

I don't really follow this.  Do you mean sourcing the functions within 
the rc file?  If so, environment variables have to be exported to be 
available in child scripts.  Additionally the scripts need to have the 
variables available if not run from the rc file.

If I misunderstand you, please elaborate.

> Interactive boot was completely removed. This was a user request
> several times in the past. I don't really have a problem with it
> going away, per se, but the addition was so simple in rc and rc.site,
> that I can't see a reason to  take away the functionality.

I did that in the name of reducing complexity, but we can revisit that 
if there is renewed interest.  The removal is mitigated by the logging 
in /run/var/bootlog, but I need to update the formatting of the output 
to that file.

> Get rid of boot_mesg all together. This was left over from a mixed 
> results attempt at international messages and long display output 
> (and line wrapping). As James mentioned the other day, it has long 
> since been abandoned by all of us involved with adding it. Both the
> echo binary and the bash built-in support the -e flag. We can choose
> /bin/bash for the schebang in the scripts, or just use /bin/echo. The
> built-in is faster, but we can guarantee that /bin/echo has the
> needed functionality if the /bin/sh symlink is changed. Personally, I
> prefer the /bin/bash schebang, but really no technical argument
> either way.

I prefer /bin/bash too, but left everything as /bin/sh (and used 
non-bash constructs).  I think there may be a problem if a non-LFS 
script uses /bin/sh and the functions use /bin/bash.

I agree that we can guarantee echo -e works and let the shell decide to 
use a builtin or /bin/echo.

> Where did /lib/boot come from? 

Alzheimer's.  I got my SS card on Saturday.  Is that a :) or a :(

> /lib/services was discussed and approved for
> the network services by 5 participants (yourself included) back in May with
> only one objection on naming which was later retracted. "boot" is also a
> poor choice for the name if the network services are stored here as they
> will be used after boot. 

I'm not real happy with /lib/services AND /lib/lsb, especially if 
lib/lsb has only one file.  How about /lib/services and a symlink from 
/lib/lsb to /lib/services?

> Same thing for /etc/sysconfig. Consensus was
> reached among participants to use /etc/default to help clean up the /etc
> directory a tiny bit, and /etc/network for network configuration scripts.

I'd like to revisit that.  The way I've done the network scripts, there 
are no subdirectories to /etc/sysconfig and all the scripts are in the 
form of ifconfig*.  On a simple system, there is only ifconfig.eth0, but 
things like ifconfig.eth0.ip2 are honored.  sysconfig stands for system 
configuration and I think more descriptive than default.

There are only a few files in /etc/sysconfig now:
   init_params
   network
   clock
   console
   createfiles
   modules
   ifconfig*

The optional init_params file is new and is intended to allow the user 
to consolidate the other files into one file.  I never liked having 
something like clock exist to define one or two environment variables.
Some of the scripts still demand their own file (that should be fixed), 
but init_params is always sourced in functions.

> Also, what about rc.site?

I'll add that back.

> Networking while certainly cleaner, still has nowhere near the functionality
> discussed on lfs-dev back in mid May which was agreed upon and improved in
> discussion by 6 participants with zero voiced objections. See below for one
> other suggestion made by Bruce that got little attention that I personally
> think should have gotten more attention.
> 
> As mentioned earlier, need --help and -h output on ifup and ifdown. A
> manual page would be a really cool addition, but certainly is not 
> required. Time would probably be better _utilized_ elsewhere, but if 
> that peaks interest, then it's cool, just be prepared for possible 
> changes, but hopefully changes will not be needed and everything can
> be placed in  the service scripts.

I intend to add -h, --help, and man pages for ifup and ifdown.

> As mentioned previously, use LSB return values and message formatting
> throughout all scripts for consistency. Use LSB defined functions as well if
> possible, but do not sacrifice ease of scripting for distro scripts to 
> do so.

> IOW: distro functions should be only wrappers to LSB functions to both ease
> the use of LSB defined functions, and make the bootscripts themselves look
> much cleaner than raw LSB scripts (which tend to look very ugly).

AFAIK, there are only 6 LSB bootscript functions:  start_daemon, 
killproc, pidofproc,  log_success_msg, log_failure_msg, log_warning_msg

> Append bootlog to /var/log/boot.log at the end of rc, fix formatting,
> and add timestamps as in any other log file. /bin/logger could be
> used to get you the correct format, though I've preferred alternate
> methods in the past.

Reformatting is already on my TODO list.  Using logger is a good idea.
Appending to /var/log/boot.log is somthing I forgot, but doing it is a 
one liner.

> Also, log
> only when the following is true:
> if [ "${PREVLEVEL}" == "N" || "${PREVLEVEL}" == "S" ]; then...
> or optionally (as was done previously):
> if [ "${RUNLEVEL}" != "6" || "${RUNLEVEL}" != "0" ]; then...

To get around a potential problem of file availability early in the boot 
process, I'm logging to /run/var/bootlog, which resides on a tmpfs.  I 
think that all boot messages should be there, even after a login prompt. 
  This is like dmesg where the kernel logs whenever messages that are 
appropriate.

There is also a problem with logging messages late in the shutdown 
process after partitions are unmounted.  I've updated the mountfs script 
for shutdown to use:

    umount -a -d -r -t notmpfs

so tmpfs mounts (/run) stays available to the end.  I suppose we could 
mark /run/var/bootlog with an end-of-boot marker and then just before 
unmounting partitions, copy everything after the mark to 
/var/log/boot.log.  That just leaves swap, localnet and halt/reboot as 
scripts not written to /var/log/boot.log.

Perhaps for shutdown, the mountfs script could be moved so the final 
sequence would be

S80swap
S90localnet
S95mountfs
S99halt/S99reboot

> Also, a little OT, but while doing major surgery anyway, didn't anybody care
> for the single line configuration format for the network that Bruce 
> suggested?
> 
> Maybe comma-space delimited would be better than colon delimited as 
> originally suggested:
> 
> # Begin eth0.conf
> #IFACE, SERVICE, PARAMS
> eth0, ipv4-static, 192.168.1.1, 24, 192.168.1.254, 192.168.1.255
> eth0, ipv6-static, 2001:270:cb5f:5::, 64, MAC
> eth0, ipv4-static-route, 192.168.2.192, 26, 192.168.1.2, 1
> # End eth0.conf
> 
> Of course, these could also be separated into 3 files named 
> 1stNIC.ipv4.conf,
> ipv6.eth0.conf, and route-to-192.168.2.192\/26.conf if desired.

I did suggest that, but decided it was simpler to use a separate file 
for each operation (but not a separate directory).  This is especially 
true if we get into wifi later in BLFS.  The idea here was to enable 
ifup/ifdown to take a wildcard.  For example, if there is

ifconfig.eth0
ifconfig.eth0.ipv4
etc

Then the user could just run `ifdown ifconfig.eth0*`

The ifconfig files require a parameter like IFACE=eth0.  We could add a 
parameter to ifup/down like '-i eth0' that would scan all the ifconfig* 
files for eth0 and also add -a parameter to bring up or down all entries.

> I think that about covers it for now. After we dig in a little 
> further, we'll see if there is anything else. To be honest, after 
> looking at that list and seeing all the presumed breakage, I'd really
> like an explanation as to how you came to the conclusion that a
> rewrite was in order as opposed to 'fixing' the scripts that were
> already written. 

I'll admit that it was a mistake.

> I understand that education was the focus, but I had
> thought the judicious use of comments in the extended functionality
> of the LSB scripts would have provided much more educational value.
> Again, not as simple, but certainly no high bar for entry either.

Let's just agree to make it right.  I'll do the changes.  I just need 
feedback like you've given here.  Thanks.

   -- Bruce
-- 
http://linuxfromscratch.org/mailman/listinfo/lfs-dev
FAQ: http://www.linuxfromscratch.org/faq/
Unsubscribe: See the above information page

Reply via email to