On Tue, May 14, 2013 at 01:22:08PM +0200, Lars Ellenberg wrote: [ on linux-ha, a lot of stuff about why I think we don't even want that feature, or at least not how it is implemented ]
This post is related, but about the implementation bugs in the current implementation as I spotted them. So this time to linux-ha-dev. If you want me to trimm the Cc, just say so ;-) So assuming we even want this feature, here are some bugs I found: ------ in lvm_vg.sh, vg_stop_tagged() probably only strip, if you are the owner, NOT if you "could" claim it. vg_tag_owner -if [ $? -ne 0 ] ; then +if [ $? = 1 ] ; then strip_tags ------ I see quite a number of lvs --options | while read line; do some_variable=whatever possibly return $SOMETHING done refer to $some_variable e.g in vg_start_exclusive(). THIS DOES NOT WORK. the whole while loop, because it reads from a pipe, executes in a subshell. variable assignments do not propagate to the main script, return values do not return from the current function, but only from the subshell, changing the exit code of that pipeline only. Note that I think most (all?) of these slipped in with the attempt to Low: LVM: Restore LVM agent portability so I guess the original is probably ok, doing result=($(subcommand)); while [ ! -z ${result[iterator]} ] ... See the example further down for how to do that same thing in portable shell. ---- # BROKEN t() { var=initial-value echo bla | while read line; do var=other-value return 42 done echo "still here" } t echo "return code: $?" echo "var=$var" ---- output: ---- still here return code: 0 var=initial-value ---- D'oh. All that fine error handling for nothing :-( Assuming they are otherwise correct, just convert all those into for some_var in $(lvs --options); do done That should do what I think you mean. ---- # OK t() { var=initial-value for line in $(echo bla); do var=other-value return 42 done echo "still here?" } t echo "return code: $?" echo "var=$var" ---- output: ---- return code: 42 var=other-value ---- For the two (or more) field case, where the original used bash array variables, you can use the positional argument array. Do something like this: t() { local result name attr # doing it this way, first declare local, # then do the "backtick assign" # preserves the exit code result=$(lvs --noheadings -o name,attr $whatever) # so you can now check the exit status, if you want to. # then assign to positional argument "array" set -- $result # if $# % 2 != 0 then panic ;-) while [ $# -ge 2 ]; do name=$1 attr=$2 shift 2 do-what-you-have-to case $attr in [rR]???????p) it-is-a-partial ;; [mM]???????p) a-partial-we-may-need-to-repair ;; *a?) : it-is-active-great ;; *) not-activated-now-what ;; esac done } shell can split lines to words and do pattern matching just fine... no need to call out to echo | awk | grep, unless word splitting and patterns are not sufficient. Or, of course, if we decide we so deperatly want this feature, maybe just ignore the /bin/bash != /bin/sh is a regression argument, use the hopefully "proven" bash code from the rgmanager branch, drop that compat commit, and ignore people whining about bash being bloated. This is Linux specific code, and I very much doubt there is a linux shipping LVM + pacemaker but no bash. --------- I have to stop this now. But if I come back later, I'll followup with whatever else I find. Cheers, Lars _______________________________________________________ Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev Home Page: http://linux-ha.org/