Re: [Qemu-devel] [PATCH 1/2] scripts/run-coverity-scan: Script to run Coverity Scan build

[Eric Blake]

On 11/13/18 1:21 PM, Peter Maydell wrote:

> > set -e...
> >
> > > +check_upload_permissions() {
> >
> >
> > ...and shell functions do NOT intuitively do what you would think. It's
> > almost always better to use explicit error checking than to rely on set -e
> > as a crutch, because then you don't get surprises.
>
> I think we had this conversation with last year's version
> of this script too :-)  As you say, the use of functions
> makes it maybe better to use explicit error checking -- but
> is there a standard syntax for that that doesn't make
> basic
>   foo
>   bar
>   baz
> "do these things in order" code look weird and require special care?
> At least with set -e you do get error checking, whereas scripts without
> it tend to just plough on regardless (look at configure, which doesn't
> use set -e but doesn't have explicit checking either).

I've seen both:

foo &&
bar &&
baz

and

foo || fail
bar || fail
baz || fail

for some variation of a 'fail' function.  But yeah, once you start 
having to worry about checking everything yourself (or realizing which 
lines don't need checking), you find out how much of a crutch 'set -e' 
tries to be, and then wonder how it ever worked (for the number of cases 
where 'set -e' does not actually catch failure, and cannot be re-enabled 
in smaller scopes).

> > > +TOOLBIN="$(cd "$COVERITY_TOOL_BASE" && echo
> > > $(pwd)/coverity_tool/cov-analysis-*/bin)"
> >
> >
> > If $CDPATH is set and $COVERITY_TOOL_BASE does not contain /, this could
> > result in garbage being prepended to TOOLBIN as output from the 'cd'. Also,
> > $PWD is nicer than $(pwd); but are you sure the glob in cov-analysis-* won't
> > select too many files?
>
> The glob is not great, but it is necessary to make the script
> robust over new versions of the tools, which put the version
> number in the cov-analysis-whatever directory name. If
> we do ever get more than one file then the "test -x" below
> will fail, and we'll be able to investigate and fix up the script.

Yeah, I think you're okay on that front.

> CDPATH sounds like a horrific misfeature designed for breaking
> scripts, so I'm not very interested in trying to work around it.
> We don't seem to worry about this in configure either.
> (I suppose we could just unset it at the start of the script.)

Autoconf 'configure' scripts do just that (unset CDPATH up front).  If 
someone ever complains that it actually broke for them, we'll make the 
fix; until then, I can live with the risk.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org