Re: [PATCH openbmc v6 07/18] initfs: update: Don't exec sh or sulogin on error just exit 1

Re: [PATCH openbmc v6 07/18] initfs: update: Don't exec sh or sulogin on error just exit 1

[Milton Miller II]

[-- Attachment #1: Type: text/html, Size: 4410 bytes --]

Re: [PATCH openbmc v6 07/18] initfs: update: Don't exec sh or sulogin on error just exit 1

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4049 bytes --]

On Sun, 2016-06-26 at 00:11 +0000, Milton Miller II wrote:
> 
> On 06/22/2016 around 11:58PM, Andrew Jeffery wrote:
> >On Wed, 2016-06-22 at 19:30 -0500, OpenBMC Patches wrote:
> >> From: "Milton D. Miller II" <miltonm@us.ibm.com>
> >> 
> >> When update was written it was exec'd from the shutdown script
> >> and hence took over pid 1.  Since exiting in that environment was
> >> a panic situation, the script instead started a rescue shell with
> >> its output presumably on the console.
> >> 
> >> The calling convention was updated to be a simple invocation in
> >> commit dbacf104885c ("obmc-initfs: run update as a sub-script")
> >> but the error handling was not updated.  That error handling is
> >> now becoming a hinderance
> >
> >hindrance
> 
> fixed
> 
> >>  findmtd() {
> >>   m=$(grep -xl "$1" /sys/class/mtd/*/name)
> >> @@ -130,7 +127,7 @@ do
> >>   if test -z "$m"
> >>   then
> >>   echo 1>&2  "Unable to find mtd partiton for ${f##*/}."
> >> - exec /bin/sh
> >> + exit 1
> >
> >Is there any value differentiating between the failure cases? I.e.
> >could we `exit 2` here instead?
> 
> The problem with differentiated exit codes is knowing what they 
> mean and keeping the callers updated with new codes.

Yes, I'm across that, hence asking if it had value rather than outright
suggesting it. I was trying to gauge how much thought had been put into
it, and from your reply I can see error handling wasn't an
afterthought.

>   After this 
> series we can recognize the following exceptional conditions 

> (1) unrecognized options (but stops checking on the first argument 
> without a leading hyphen, additional arguments are accepted and 
> ignored), 

> (2) can't match image suffix to a mtd label 

> (3) image too big for partition 

> (4) mtd or a sub-mtd partition is in /proc/self/mounts 

> (5) unknown error from flash_cp (could be a erase, write, or verify 
> error, or its size check) 

> (6) trying to restore without any saved files 

> (7) bad entry in whitelist 

> (8) trying to flash with no images.   


> Of these, 
> (7) and (8) print a message to stdout, (5) and (6) are totally silent, 
> while (1), (2), (3), and (4) exit 1 on first encounter stopping further 
> checks.  I have some rational for the choices, mostly I want to flash 
> as much as possible if the set of images passed a first scan to 
> plausible success (and the arg parsing is to allow future extension 
> to pass the files to flash) and not caring between whitelist didn't 
> match anything vs whitelist entries are bad when flashing is the 
> primary function.  Unknown error (5) is silent because I already had 
> remove on success and didn't bother to add code to remember the 
> error while wanting as much success as possible.
> 
> I could see differentiating between unrecognized options (1), image 
> file doesn't have an mtd or is too big for the mtd (2 or 3),

I would have thought reporting (2) and (3) would be useful in that they
are simple to detect and the problem is obvious (though the solution
isn't necessarily obvious).

>  and flash 
> update error (5), but what would be the use case?  If from a user they 
> would run --help or read the error message and correct, and hopefully 
> calls from REST would return the error.  The bad whitelist (7) and no 
> files saved before trying to restore or copy files (6) will be debugged 
> when someone notices the contents not there after an upgrade (the 
> default is currently to save and restore).  The check for a busy 
> mount (4) vs check for size and name (3) can be handled by calling 
> again with --ignore-mount if the script wants to know if deferred update 
> is likely to work.
> 
> So for now I will stay with one error and only report when checks fail.  
> Let me know if you think an exit code for (8) is important and I'll add 
> that.

I don't feel strongly about (8), it's fine as is.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH openbmc v6 07/18] initfs: update: Don't exec sh or sulogin on error just exit 1

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]

On Wed, 2016-06-22 at 19:30 -0500, OpenBMC Patches wrote:
> From: "Milton D. Miller II" <miltonm@us.ibm.com>
> 
> When update was written it was exec'd from the shutdown script
> and hence took over pid 1.  Since exiting in that environment was
> a panic situation, the script instead started a rescue shell with
> its output presumably on the console.
> 
> The calling convention was updated to be a simple invocation in
> commit dbacf104885c ("obmc-initfs: run update as a sub-script")
> but the error handling was not updated.  That error handling is
> now becoming a hinderance

hindrance

>  to use from additional environments so
> change it.
> 
> Signed-off-by: Milton Miller <miltonm@us.ibm.com>
> ---
>  .../obmc-phosphor-initfs/files/obmc-update.sh      | 23 ++++++----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh b/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh
> index aa8fd89..f8e551c 100755
> --- a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh
> +++ b/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh
> @@ -2,9 +2,6 @@
>  
>  echo update: "$@"
>  
> -export PS1=update-sh#\ 
> -# exec /bin/sh
> -
>  cd /
>  if ! test -r /proc/mounts || ! test -f /proc/mounts
>  then
> @@ -21,12 +18,12 @@ then
>  	mkdir -p /dev
>  	mount -t devtmpfs dev dev
>  fi
> -while grep mtd /proc/mounts
> -do
> +
> +if grep mtd /proc/mounts
> +then
>  	echo 1>&2 "Error: A mtd device is mounted."
> -	sulogin
> -	# exec /bin/sh
> -done
> +	exit 1
> +fi
>  
>  findmtd() {
>  	m=$(grep -xl "$1" /sys/class/mtd/*/name)
> @@ -130,7 +127,7 @@ do
>  	if test -z "$m"
>  	then
>  		echo 1>&2  "Unable to find mtd partiton for ${f##*/}."
> -		exec /bin/sh
> +		exit 1

Is there any value differentiating between the failure cases? I.e.
could we `exit 2` here instead?

It is just a thought and I am not overly fussed by it.


>  	fi
>  done
>  
> @@ -173,11 +170,3 @@ then
>  fi
>  
>  exit
> -
> -# NOT REACHED without edit
> -# NOT REACHED without edit
> -
> -echo "Flash completed.  Inspect, cleanup and reboot -f to continue."
> -
> -export PS1=update-sh#\ 
> -exec /bin/sh
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH openbmc v6 07/18] initfs: update: Don't exec sh or sulogin on error just exit 1

[OpenBMC Patches]

From: "Milton D. Miller II" <miltonm@us.ibm.com>

When update was written it was exec'd from the shutdown script
and hence took over pid 1.  Since exiting in that environment was
a panic situation, the script instead started a rescue shell with
its output presumably on the console.

The calling convention was updated to be a simple invocation in
commit dbacf104885c ("obmc-initfs: run update as a sub-script")
but the error handling was not updated.  That error handling is
now becoming a hinderance to use from additional environments so
change it.

Signed-off-by: Milton Miller <miltonm@us.ibm.com>
---
 .../obmc-phosphor-initfs/files/obmc-update.sh      | 23 ++++++----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh b/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh
index aa8fd89..f8e551c 100755
--- a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh
+++ b/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh
@@ -2,9 +2,6 @@
 
 echo update: "$@"
 
-export PS1=update-sh#\ 
-# exec /bin/sh
-
 cd /
 if ! test -r /proc/mounts || ! test -f /proc/mounts
 then
@@ -21,12 +18,12 @@ then
 	mkdir -p /dev
 	mount -t devtmpfs dev dev
 fi
-while grep mtd /proc/mounts
-do
+
+if grep mtd /proc/mounts
+then
 	echo 1>&2 "Error: A mtd device is mounted."
-	sulogin
-	# exec /bin/sh
-done
+	exit 1
+fi
 
 findmtd() {
 	m=$(grep -xl "$1" /sys/class/mtd/*/name)
@@ -130,7 +127,7 @@ do
 	if test -z "$m"
 	then
 		echo 1>&2  "Unable to find mtd partiton for ${f##*/}."
-		exec /bin/sh
+		exit 1
 	fi
 done
 
@@ -173,11 +170,3 @@ then
 fi
 
 exit
-
-# NOT REACHED without edit
-# NOT REACHED without edit
-
-echo "Flash completed.  Inspect, cleanup and reboot -f to continue."
-
-export PS1=update-sh#\ 
-exec /bin/sh
-- 
2.9.0