bvt's backtrace

As a result of latest discussions with Diana Coman and Mircea Popescu, I have made some changes to the the v.sh shell wrapper. The main requests for changes were:

• Stop using "-" as a synonym for "all vpatches". Possibly select all vpatches for pressing if the "leafs" argument is not provided.
• Add a command for visual inspection of the vtree.
• Have a verbose mode, make sure that the tool output is machine-readable and pipeable.
• There was also a discussion about code style: whether the patches, seals, and wot directories should appear verbatim in the source or indirectly via corresponding variables.

As the discussion was going on, I took time to pass v.sh through a tool called shellcheck¹, which intends to detect insecure or incorrect shell usage, and did give some useful, albeit minor, suggestions. Those are, with the examples from vpatch:

• Missing quoting of variables. It's hard to foresee how this can cause probles, but than again if the robustness of the tool can be improved without extra complexity, I'm all for it.

  @@ -52,22 +52,22 @@
   trap cleanup TERM INT
  
   exit_msg() {
  -	echo $1
  +	echo "$1"
   	cleanup
   }
  
   verify() {
   	vpatchpath="$1"
  -	vpatch="$(basename $vpatchpath)"
  +	vpatch=$(basename "$vpatchpath")
   	signers=""
  
   	checkdir seals
   	for i in seals/$vpatch.*.sig; do
  -		signature="$(basename $i | sed 's/.sig$//g')"
  +		signature=$(basename "$i" | sed 's/.sig$//g')
   		signer="${signature#$vpatch.}"
   		if ! gpg --homedir "$vgpgdir" --logger-fd 1 --batch \
  

• Bad order of redirections. This is a useful one, as I missed it during development.

   		if ! gpg --homedir "$vgpgdir" --logger-fd 1 --batch \
   			--keyid-format=long \
  -			--verify "$i" "$vpatchpath" 2>&1 >/dev/null; then
  +			--verify "$i" "$vpatchpath" >/dev/null 2>&1; then
   			exit_msg "Bad signature $i for $vpatch"
   		fi
   		signers="$signers $signer"
  

• An unused variable.

  @@ -116,8 +116,7 @@
   vpatchflow() {
   	[ -z "$*" ] && exit_msg "No leaves selected."
   	filterall | internall >"$filtered"
  -	selected=""
  -	if [ "$*" == "-" ]; then
  +	if [ "$*" = "-" ]; then
   		echo "S" >"$selfile"
   	else
   		echo $* | tr ' ' '\n' | vpatchnos | sed -e 's/^/s /g' - >"$selfile"
  

• Use of $* instead of "$@". In this suggestion was not helpful, because after considering code end-to-end, it became clear, that vpatches with spaces in the filenames would require more extensive changes to be supported. Maybe in the next iteration, if it is worth the extra complexity. Piping stuff with space in a name is a mess.

  if [ "$1" = "flow" ]; then
  	shift
  
  	vpatchflow $*
  
  	echo "Press order:"
  	awk '$1 == "P" {print $2}' < "$topo" | vpatchnames
  
  	vpatchflowerr
  	cleanup
  fi
  

What this tool did not catch, was that "==" is nonstandard syntax, even though commonly supported. However, I did find a system under my control where this problem surfaced. So fix was applied in this vpatch as well.

@@ -224,7 +223,7 @@
 	cleanup
 fi;

-if [ "$1" == "flow" ]; then
+if [ "$1" = "flow" ]; then
 	shift

 	vpatchflow $*

In general, I'd say this this tool is useful for learning, but as with any other tool, its suggestions cannot be accepted without thinking. What is sane about it is the set of explanations of corrections (example) - perhaps those are worth much more then the tool itself.

curl 'http://bvt-trace.net/vpatches/vtools_vsh_shellcheck.vpatch' > vtools_vsh_shellcheck.vpatch
curl 'http://bvt-trace.net/vpatches/vtools_vsh_shellcheck.vpatch.bvt.sig' > vtools_vsh_shellcheck.vpatch.bvt.sig

---

The next set of changes come from discussion with Diana Coman and Mircea Popescu:

• "$allkeyword" is introduced, to replace the usage of "-" to signify 'all vpatches', but now set to "all".
• Not specifying any vpatch now selects all vpatches (equivalent to "$allkeyword").
• A command for visual inspection of vpatch tree ("vtree") is added. It prints the flow, indented by the number of antecedents.
• A as a result of discussion between Diana and Mircea, I decided not to change wot/patches/seals directories with "$wotdir", etc. If this ever needs to be changed, I'll introduce the variables for them. If not, they will stay like that.

I would like to take more time to think about the verbose flag, so in meantime I have unsuppressed output of vpatch. So far my idea of how this will be implemented:

1. The verbose mode is enabled by setting the VERBOSE environment variable.
2. set -x will be executed at the very beginning of a script, enabling the echoing of each command executed in v.sh.
3. All output currently redirected to /dev/null will go to stdout.
4. Perhaps, I will also avoid deleting the temporary directory, for better debugging.

What do you think?

For easier piping, I did some changes in this very vpatch:

1. The tool exits with non-zero exit codes on errors.
2. There are no headers in case of correct output.
3. The error reporting is redirected to stderr.

GNU tools typically check whether the stdout is a tty (literally isatty(3)) - first, the equivalent is not available in Shell, to the best of my knowledge. Second, IMO it's better to massage the output into being usable for both human and machine, than to rely on DWIM.

--- a/vtools/src/vflow.adb 400069490b8212c36d75f27a9935f5017ad6bdb49c9f520293082a4dcc4a1e902fe3733cf34c4356c8a0c315905b8534236ad9b1557a32ec501742693f644514
+++ b/vtools/src/vflow.adb e6faf838d6886b05028f7483b7ff738584217d570a2a894fbe91380614a576f25952120f7fa5c1dff3bd6b16e53364cb5859ed2407412ab121c1c76d0826044c
@@ -366,6 +366,8 @@
 	 for I in Vpatch_Order'Range loop
 	    if Selected_Vpatches(Vpatch_Order(I)) then
 	       Put_Line("P " & Integer'Image(Vpatch_Order(I)));
+	    else
+	       Put_Line("p " & Integer'Image(Vpatch_Order(I)));
 	    end if;
 	 end loop;
       end Print_Flow_Or_Conflicts;

First, changes to vflow press order output. Previously, the tool would print only the patches in the flow of selected leaves. Now, the full flow is printed, and the patches in the flow are printed with capital "P", not in the flow - with lowercase. This change is necessary for the ascii flow visualization.

@@ -2,6 +2,8 @@

 set -e

+allkeyword="all"
+
 vgpgdir=""
 vpatches=""
 filtered=""

The keyword for "all vpatches" is introduced.

@@ -18,13 +20,14 @@
 v descendants patch       -- list patch descendants
 v origin hash             -- find patch that introduced the hash
 v press directory leaves  -- press all patches up to head
+v vtree leaves            -- print the vtree structure for a press
 EOF
 	exit 0
 }

 checkdir() {
 	if [ ! -d "$1" ]; then
-		echo "$1 directory not found"
+		>&2 echo "$1 directory not found"
 		kill -TERM 0
 	fi
 }

A help entry for "vtree" is added to usage, and error messages are redirected to stderr using >&2.

@@ -47,13 +50,20 @@
 	if [ ! -z "$vgpgdir" ]; then
 		rm -rf "$vgpgdir"
 	fi
-	exit 0
+	exit "$1"
+}
+exit_success() {
+	cleanup 0
 }
-trap cleanup TERM INT
+
+fail() {
+	cleanup 1
+}
+trap fail TERM INT

 exit_msg() {
-	echo "$1"
-	cleanup
+	>&2 echo "$1"
+	cleanup 1
 }

 verify() {

cleanup now takes an argument - exit code. Two functions are introduced to: exit_success to invoke cleanup with argument 0, and fail to exit with code 1. On signals, exit with code 1. Also, use stderr instead of stdout in exit_msg wrapper.

@@ -105,7 +115,8 @@

 vpatchnos() {
 	awk -v VF="$vpatches" '
-FILENAME==VF {a[$1]=$2} FILENAME=="-" {print a[$1]}' "$vpatches" -
+FILENAME==VF {a[$1]=$2}
+FILENAME=="-" {if (!($1 in a)) {print "not found:",$1} else {print a[$1]}}' "$vpatches" -
 }

 vpatchnames() {

Additional error reporting in vpatchnos: vpatchnos is directly handling user input, and as such it now reports the cases when vpatch specified by user does not exit. The in-band signaling is horrible, of course, but awk lacks an option to print to stderr, which I would have used otherwise.

@@ -114,12 +125,15 @@
 }

 vpatchflow() {
-	[ -z "$*" ] && exit_msg "No leaves selected."
 	filterall | internall >"$filtered"
-	if [ "$*" = "-" ]; then
+	if [ "$*" = "$allkeyword" ] || [ -z "$*" ]; then
 		echo "S" >"$selfile"
 	else
-		echo $* | tr ' ' '\n' | vpatchnos | sed -e 's/^/s /g' - >"$selfile"
+		echo $* | tr ' ' '\n' | vpatchnos >"$selfile"
+		if >&2 grep "not found:" "$selfile"; then
+			fail
+		fi
+		sed -i -e 's/^/s /g' "$selfile"
 	fi
 	cat "$filtered" "$selfile" | \
 		vflow $(wc -l <"$filtered") $(wc -l <"$vpatches") >"$topo"

Now, empty arguments to vpatchflow are considered to be synonym for all vpatches; errors reported by vpatchnos are conveyed to user via stderr.

@@ -133,17 +147,17 @@
 END {if (err != 0) exit err; if (ok != 1) exit 3; exit 0;}' <"$topo"
     exitcode="$?"
     if [ "$exitcode" -eq "1" ]; then
-	    printf "\nConflicting leaves selected:\n"
-	    awk '$1 == "C" {print $2}' < "$topo" | vpatchnames | sort | uniq
-	    cleanup
+	    >&2 printf "\nConflicting leaves selected:\n"
+	    >&2 awk '$1 == "C" {print $2}' < "$topo" | vpatchnames | sort | uniq
+	    fail
     elif [ "$exitcode" -eq "2" ]; then
-	    printf "\nConflicting leaves selected:\n"
-	    awk '{print $2}' < "$topo" | vpatchnames
-	    cleanup
+	    >&2 printf "\nConflicting leaves selected:\n"
+	    >&2 awk '{print $2}' < "$topo" | vpatchnames
+	    fail
     elif [ "$exitcode" -eq "3" ]; then
-	    echo "Unknown error. Investigate in $vgpgdir:"
-	    cat "$topo"
-	    exit 0 # no cleanup
+	    >&2 echo "Unknown error. Investigate in $vgpgdir:"
+	    >&2 cat "$topo"
+	    exit 1 # no cleanup
     fi
 }

Use stderr in for error reporting in vpatchflowerr as well.

@@ -153,14 +167,14 @@

 if [ "$1" = "wot" ]; then
 	gpg --homedir "$vgpgdir" --logger-fd 1 --batch --keyid-format=long -k
-	cleanup
+	exit_success
 fi

 verifyall

 if [ "$1" = "patches" ]; then
 	cat "$vgpgdir"/signers.txt
-	cleanup
+	exit_success
 fi

 if [ "$1" = "graph" ]; then
@@ -170,7 +184,7 @@
 	svgfile="$1"
 	shift

-	vpatchflow -
+	vpatchflow
 	cat >"$dotfile" <> "$dotfile"
 	dot -Tsvg "$dotfile" -o "$svgfile"
 	# dot -Tcmapx $dotfile -o $svgfile.html-map
-	cleanup
+	exit_success
 fi

 if [ "$1" = "antecedents" ]; then

Use new wrappers for cleanup 0 all over the place.

 if [ "$1" = "antecedents" ]; then
@@ -190,13 +204,14 @@
 	vpatch="$1"
 	shift

-	vpatchflow -
+	vpatchflow
 	vno=$(awk -v V="$vpatch" '$1==V {print $2}' < "$vpatches")
+	[ -z "$vno" ] && exit_msg "not found: $vpatch"
 	awk -v N="$vno" '
 ($1 == "I" || $1 == "d" ) && $2 == N {a[$3]=1}
 ($1 == "P") {if ($2 in a) {print $2}}
 ' "$topo" | vpatchnames
-	cleanup
+	exit_success
 fi

 if [ "$1" = "descendants" ]; then
@@ -204,13 +219,14 @@
 	vpatch="$1"
 	shift

-	vpatchflow -
+	vpatchflow
 	vno=$(awk -v V="$vpatch" '$1==V {print $2}' < "$vpatches")
+	[ -z "$vno" ] && exit_msg "not found: $vpatch"
 	awk -v N="$vno" '
 ($1 == "I" || $1 == "d" ) && $3 == N {a[$2]=1}
 ($1 == "P") {if ($2 in a) {print $2}}
 ' "$topo" | vpatchnames
-	cleanup
+	exit_success
 fi

 if [ "$1" = "origin" ]; then

antecedents and descendants commands are changed to invoke vpatchflow without arguments, get extra error reporting, and use the cleanup 0 wrappers.

 if [ "$1" = "flow" ]; then
 	shift

 	vpatchflow $*
+	vpatchflowerr

-	echo "Press order:"
 	awk '$1 == "P" {print $2}' < "$topo" | vpatchnames

-	vpatchflowerr
-	cleanup
+	exit_success
 fi

flow has lost its header, reports errors in the correct place, and uses the new cleanup wrapper.

 if [ "$1" = "press" ]; then
 	shift
 	dir="$1"
+	[ -z "$dir" ] && exit_msg "Provide press directory"
 	shift

 	[ -d "$dir" ] && exit_msg "Directory $dir already exists"
@@ -249,12 +265,28 @@
 	awk '$1 == "P" {print $2}' < "$topo" | vpatchnames | while read i; do
 		i=$(readlink -f "$i")
 		cd "$dir"
-		vpatch <"$i" >/dev/null
+		vpatch <"$i"
 		cd - >/dev/null
 	done
-	cleanup
+	exit_success
+fi

press has extra error checking: user must provide the directory name, vpatch output is unsuppressed, also, use the new wrapper.

+
+if [ "$1" = "vtree" ]; then
+	shift
+
+	vpatchflow "$@"
+	awk -v VF="$vpatches" '
+function repeat(s,n) {i=0; while (i++ < n) {printf("%s",s);}}
+FILENAME==VF {n[$2]=$1}
+FILENAME=="-" && ($1 == "I" || $1 == "d" ) {a[$2]++;b[$3]=1;}
+FILENAME=="-" && ($1 == "P" || $1 == "p") {
+   if (!($2 in a)) {a[$2]=0};
+   repeat(" ", a[$2]);
+   printf("%s%s%s\n",n[$2], ($2 in b) ? "" : " (*)", ($1 == "P") ? " +" : "" );
+}' "$vpatches" - <"$topo"
+	exit_success
 fi

The vtree command prints the flow, one line per vpatch, indented by the number of spaces equal to the number of antecedents of the vpatch. Additionally, it prints "(*)" mark for the leafs, and "+" mark for vpatches selected for press.
The work is done in an awk script:

1. repeat prints the designated number of spaces.
2. the next line reads the vpatches from file into n array.
3. then, the script reads from stdin:
   1. if it reads "I descendant antecedent" or "I descendant antecedent" command, it increments the number of antecedents of the descendant in the a array, and adds the antecedent into the set of non-leafs in the b array.
   2. if it reads "P vpatch" or "p vpatch" commands, it sets the count to zero (for genesis), prints the indentation, and outputs the vpatch name, leaf mark, and selection-for-press mark to stdout.

-echo "Unknown command: $1"

-cleanup
+>&2 echo "Unknown command: $1"
+fail

Unknown command is also reported better (stderr & exit code).

The vpatches are here:

curl 'http://bvt-trace.net/vpatches/vtools_vsh_ascii_visualization.vpatch' > vtools_vsh_ascii_visualization.vpatch
curl 'http://bvt-trace.net/vpatches/vtools_vsh_ascii_visualization.vpatch.bvt.sig' > vtools_vsh_ascii_visualization.vpatch.bvt.sig

Enjoy!

1. Implemented in Haskell, so probably belongs to a machine dedicated for software insanities. [↩]

This article was published on Sunday, March 8th, 2020 at 13:23 and is filed under Ada, Linux. You can follow any responses to this entry through the RSS 2.0 feed. You can leave a response, or trackback from your own site.