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

[Andrew Jeffery]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160627/8d8bf6f8/attachment.sig>