Re: V2 [PATCH 2/3] Add pidofproc to ${sysconfdir}/init.d/functions

[Darren Hart <dvhart@linux.intel.com>]

On 05/16/2011 08:19 AM, Robert Yang wrote:
> Add pidofproc to ${sysconfdir}/init.d/functions, this is used for
> getting the pid of the process. It uses pidof to implement currently, it
> may also use the pidfile or ps to implement in the future.
> 
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> ---
>  .../initscripts/initscripts-1.0/functions          |   32 ++++++++++++++++++-
>  1 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/meta/recipes-core/initscripts/initscripts-1.0/functions b/meta/recipes-core/initscripts/initscripts-1.0/functions
> index 689fd32..6a51b3b 100644
> --- a/meta/recipes-core/initscripts/initscripts-1.0/functions
> +++ b/meta/recipes-core/initscripts/initscripts-1.0/functions
> @@ -4,11 +4,39 @@
>  #               shell scripts in the /etc/init.d directory.
>  #
>  
> +# NOTE: The pidofproc () doesn't support the process which is a script unless
> +#       the pidof supports "-x" option. If you want to use it for such a
> +#       process:
> +#       1) If there is no "pidof -x", replace the "pidof $1" with another
> +#          command like(for core-image-minimal):
> +#            ps | awk '/'"$1"'/ {print $1}'
> +#       Or
> +#       2) If there is "pidof -x", replace "pidof" with "pidof -x".

Please document the variables to newly created functions, for shell
functions, something like:

# pidofproc - print the pid of a process
# $1: the name of the process

> +pidofproc () {
> +        local pid status

This patch uses 8 spaces to indent while the existing file uses 4. The
state of shell script whitespace in the repository is a bit of a
disaster. We can't really go through it all and just correct it for the
sake of consistent whitespace, but when modifying shell scripts, please
do the following:

1) Cleanup the whitespace in the affected file
   Do this before making functional changes to it and submit the
   cleanup as a separate patch.
2) Indent with tabs
3) Do not indent the cases in a case block


> +
> +        status=0
> +        # pidof output null when no program is running, so no "2>/dev/null".
> +        pid=`pidof $1` || status=$?
> +        case $status in

The following is just my opinion, you may ignore it if you disagree:

I find the "do_this || die" syntax to be awkward, and have seen it lead
to non-intuitive errors. For testing the success of a command, consider
removing the status variable and using:

pid=$(pidof $1)
case $? in



> +          0)
> +                echo $pid
> +                return 0
> +                ;;
> +          127)
> +                echo "ERROR: command pidof not found" >&2
> +                exit 127
> +                ;;
> +          *)
> +                return $status
> +                ;;
> +        esac
> +}
> +
>  machine_id() {		# return the machine ID
>      awk 'BEGIN { FS=": " } /Hardware/ { gsub(" ", "_", $2); print tolower($2) } ' </proc/cpuinfo
>  }
>  
>  killproc() {		# kill the named process(es)
> -    pid=`/bin/pidof $1`
> -    [ "$pid" != "" ] && kill $pid
> +    pid=`pidofproc $1` && kill $pid
>  }

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel