[ClusterLabs] ocf scripts shell and local variables

[Dejan Muhamedagic]

On Tue, Aug 30, 2016 at 06:53:24PM +0200, Lars Ellenberg wrote:
> On Tue, Aug 30, 2016 at 06:15:49PM +0200, Dejan Muhamedagic wrote:
> > On Tue, Aug 30, 2016 at 10:08:00AM -0500, Dmitri Maziuk wrote:
> > > On 2016-08-30 03:44, Dejan Muhamedagic wrote:
> > > 
> > > >The kernel reads the shebang line and it is what defines the
> > > >interpreter which is to be invoked to run the script.
> > > 
> > > Yes, and does the kernel read when the script is source'd or executed via
> > > any of the mechanisms that have the executable specified in the call,
> > > explicitly or implicitly?
> > 
> > I suppose that it is explained in enough detail here:
> > 
> > https://en.wikipedia.org/wiki/Shebang_(Unix)
> > 
> > In particular:
> > 
> > https://en.wikipedia.org/wiki/Shebang_(Unix)#Magic_number
> > 
> > > >None of /bin/sh RA requires bash.
> > > 
> > > Yeah, only "local".
> > 
> > As already mentioned elsewhere in the thread, local is supported
> > in most shell implementations and without it we otherwise
> > wouldn't to be able to maintain software. Not sure where local
> > originates, but wouldn't bet that it's bash.
> 
> Let's just agree that as currently implemented,
> our collection of /bin/sh scripts won't run on ksh as shipped with
> solaris (while there likely are ksh derivatives in *BSD somewhere
> that would be mostly fine with them).

I can recall people running some of the BSD systems and they
didn't complain about resource-agents.

> And before this turns even more into a "yes, I'm that old, too" thread,

I guess that is in human nature.

> may I suggest to document that we expect a
> "dash compatible" /bin/sh, and that we expect scripts
> to have a bash shebang (or as appropriate) if they go beyond that.

That is how it has been in resource-agents: POSIX compatible with
the addition of "local". Anyway, every script must have a #! line
which states the right interpreter.

> Then check for incompatible shells in ocf-shellfuncs,
> and just exit early if we detect incompatibilities.
> 
> For a suggestion on checking for a proper "local" see below.
> (Add more checks later, if someone feels like it.)
> 
> Though, if someone rewrites not the current agents, but the "lib/ocf*"
> help stuff to be sourced by shell based agents in a way that would
> support RAs in all bash, dash, ash, ksh, whatever,
> and the result turns out not too much worse than what we have now,
> I'd have no problem with that...
> 
> Cheers,
> 
>     Lars
> 
> 
> And for the "typeset" crowd,
> if you think s/local/typeset/ was all that was necessary
> to support function local variables in ksh, think again:
> 
> ksh -c '
> 	function a {
> 		echo "start of a: x=$x"
> 		typeset x=a
> 		echo "before b: x=$x"
> 		b
> 		echo "end of a: x=$x"
> 	}
> 	function b {
> 		echo "start of b: x=$x ### HAHA guess this one was unexpected to all but ksh users"
> 		typeset x=b
> 		echo "end of b: x=$x"
> 	}
> 	x=x
> 	echo "before a: x=$x"
> 	a
> 	echo "after a: x=$x"
> '
> 
> Try the same with bash.

:)

> Also remember that sometimes we set a "local" variable in a function
> and expect it to be visible in nested functions, but also set a new
> value in a nested function and expect that value to be reflected
> in the outer scope (up to the last "local").

I hope that this wasn't (ab)used much, it doesn't sound like it
would be easy to follow.

> diff --git a/heartbeat/ocf-shellfuncs.in b/heartbeat/ocf-shellfuncs.in
> index 6d9669d..4151630 100644
> --- a/heartbeat/ocf-shellfuncs.in
> +++ b/heartbeat/ocf-shellfuncs.in
> @@ -920,3 +920,37 @@ ocf_is_true "$OCF_TRACE_RA" && ocf_start_trace
>  if ocf_is_true "$HA_use_logd"; then
>  	: ${HA_LOGD:=yes}
>  fi
> +
> +# We use a lot of function local variables with the "local" keyword.
> +# Which works fine with dash and bash,
> +# but does not work with e.g. ksh.
> +# Fail cleanly with a sane error message,
> +# if the current shell does not feel compatible.
> +
> +__ocf_check_for_incompatible_shell_l2()
> +{
> +	[ $__ocf_check_for_incompatible_shell_k = v1 ] || return 1
> +	local __ocf_check_for_incompatible_shell_k=v2
> +	[ $__ocf_check_for_incompatible_shell_k = v2 ] || return 1
> +	return 0
> +}
> +
> +__ocf_check_for_incompatible_shell_l1()
> +{
> +	[ $__ocf_check_for_incompatible_shell_k = v0 ] || return 1

If there's no "local" and that in the function below fails, won't
this produce a syntax error (due to __ocf_..._k being undefined)?

> +	local __ocf_check_for_incompatible_shell_k=v1
> +	__ocf_check_for_incompatible_shell_l2
> +	[ $__ocf_check_for_incompatible_shell_k = v1 ] || return 1
> +	return 0
> +}
> +
> +__ocf_check_for_incompatible_shell()
> +{
> +	local __ocf_check_for_incompatible_shell_k=v0
> +	__ocf_check_for_incompatible_shell_l1
> +	[ $__ocf_check_for_incompatible_shell_k = v0 ] && return 0
> +	ocf_exit_reason "Current shell seems to be incompatible. We suggest dash or bash (compatible)."
> +	exit $OCF_ERR_GENERIC
> +}
> +
> +__ocf_check_for_incompatible_shell

Looks good otherwise. If somebody's willing to test it on
solaris...

Thanks,

Dejan

> 
> _______________________________________________
> Users mailing list: Users at clusterlabs.org
> http://clusterlabs.org/mailman/listinfo/users
> 
> Project Home: http://www.clusterlabs.org
> Getting started: http://www.clusterlabs.org/doc/Cluster_from_Scratch.pdf
> Bugs: http://bugs.clusterlabs.org