On Thu, 2021-03-18 at 14:32 -0400, Scott Andrews wrote:
> On Thu, 18 Mar 2021 11:51:09 -0500
> Bruce Dubbs <bruce.du...@gmail.com> wrote:
> 
> > On 3/18/21 11:21 AM, Scott Andrews wrote:
> > > 
> > > I am presently looking at and working on the LFS boot scripts.
> > > They are in my opinion very rough state.
> > > 
> > > I am going to clean them up and use the following format for all
> > > of the individual scripts that will be used on my systems as
> > > follows:
> > > 
> > > Shebang line:           #!/bin/bash
> > > Comment Title block:    the purpose of the script
> > > Global variables:       defined here
> > > Local variables:        not defined in functions
> > > Source scripts:         all outside scripts sourced here
> > > Functions:              all functions defined here
> > > Mainline:               main body
> > > Cleanup:                any cleanup that needs to be done
> > > Exit:                   script terminates
> > > 
> > > I will add comments to explain the goal/purpose of the script and
> > > also what each function does. I am going to rewrite some of the
> > > scripts so they will be self sufficient as possible.
> > > 
> > > I will add ipv6 support using a service file much like
> > > ivp4-static. This should allow ipv6 to be used on both dual
> > > stacked systems and system that are ipv4 or ipv6 only.
> > > 
> > > I am also going to build to test them before placing them onto my
> > > working servers.
> > > 
> > > Is there any interest by LFS on doing this or am I just wasting my
> > > time posting here?  
> > 
> > What you propose seems to be mostly documentation, but other
> > changes may be appropriate.  I suggest you do a couple and let us
> > review them. Then, with constructive feedback, the rest of the LFS
> > scripts.
> > 
> > I'll note though that you are the only one giving feedback and the 
> > scripts have only had minor changes since at least 2011.
> 
> example follows.........

Some shell style comments:
> 
> remove SCRIPT_STAT from init-functions
> 
> Correctly write check_script_status in rc
> 
> function check_script_status {
>         local script=${1}
It's preferable to use quotes around any variable you do not know in advance:
script="$1"

>         SCRIPT_STAT="0"
But here it is not needed: SCRIPT_STAT=0

>         if [ ! -f ${script} ]; then
Same here. Moreover, if script is empty, you'll get an error: 
if [ ! -f "${script}" ]; then

>                 log_warning_msg "${script} is not a valid file."
>                 SCRIPT_STAT=retval="1"
=1. Note that "echo $SCRIPT_STAT" would print "retval=1", and that retval is
empty after this statement. Is it what you want?
Shouldn't there be an exit or return here? If $script is not a file, there is
little chance it is executable.

>         fi
>         if [ ! -x ${script} ]; then
if [ ! -x "${script}" ]; then ...

>                 log_warning "${script} is not executable."
>                 SCRIPT_STAT=retval="1"
=1, and same note as above.
>         fi
>         return
> }
> 
> #       Check script for file and executable
> check_script_status
> if [ "1" = ${SCRIPT_STAT} ]; then continue; fi
You'd better quote ${SCRIPT_STAT} instead of 1. And, as said above, SCRIPT_STAT
may contain "retval=1".


> 
> That fixes the issue but I would rewrite check_script_status to
> return true or false then then do this
> 
> function check_script_status {
>         local script=${1}
>         local retval="0"
>         if [ ! -f ${script} ]; then
>                 log_warning_msg "${script} is not a valid file."
>                 retval="1"
>         fi
>         if [ ! -x ${script} ]; then
>                 log_warning "${script} is not executable."
>                 retval="1"
>         fi
>         [ "0" = ${retval} ] && return 0 || return 1
Why not `return retval'?

> }
> 
> if [ check_script_status ]; then continue; fi
No brackets here. Brackets introduce predicates like -f, -x, or (in)equality.
Testing the return of a command (a function here) is just done with `if
<command>'. Note that `if [ check_script_status = 0 ]' wouldn't work either.

Also, it seems to me that the logic would be `if ! check_script_status; then
continue; fi'

> 
> or simply 
> 
> [ check_script_status ] || continue
> 
without brackets...

Pierre

-- 
http://lists.linuxfromscratch.org/listinfo/lfs-support
FAQ: http://www.linuxfromscratch.org/blfs/faq.html
Unsubscribe: See the above information page

Do not top post on this list.

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

http://en.wikipedia.org/wiki/Posting_style

Reply via email to