Parse and return fwup ops errors

This turns on fwup output framing so that the error output is clean even
if there are warnings.

.credo.exs

@@ -5,6 +5,7 @@
       name: "default",
       strict: true,
       checks: [
+        {CredoBinaryPatterns.Check.Consistency.Pattern},
         {Credo.Check.Refactor.MapInto, false},
         {Credo.Check.Warning.LazyLogging, false},
         {Credo.Check.Readability.LargeNumbers, only_greater_than: 86400},

lib/nerves_runtime/fwup_ops.ex

@@ -43,7 +43,7 @@ defmodule Nerves.Runtime.FwupOps do
   def revert(opts \\ []) do
     reboot? = Keyword.get(opts, :reboot, true)
 
-    with :ok <- run_fwup("revert", opts) do
+    with {:ok, _} <- run_fwup("revert", opts) do
       if reboot? do
         Nerves.Runtime.reboot()
       else
@@ -61,7 +61,7 @@ defmodule Nerves.Runtime.FwupOps do
   """
   @spec prevent_revert(options()) :: :ok | {:error, reason :: any}
   def prevent_revert(opts \\ []) do
-    run_fwup("prevent-revert", opts)
+    run_fwup("prevent-revert", opts) |> ignore_success_results()
   end
 
   @doc """
@@ -75,7 +75,7 @@ defmodule Nerves.Runtime.FwupOps do
   """
   @spec validate(options()) :: :ok | {:error, reason :: any}
   def validate(opts \\ []) do
-    run_fwup("validate", opts)
+    run_fwup("validate", opts) |> ignore_success_results()
   end
 
   @doc """
@@ -92,7 +92,7 @@ defmodule Nerves.Runtime.FwupOps do
   def factory_reset(opts \\ []) do
     reboot? = Keyword.get(opts, :reboot, true)
 
-    with :ok <- run_fwup("factory-reset", opts) do
+    with {:ok, _} <- run_fwup("factory-reset", opts) do
       if reboot? do
         # Graceful shutdown can cause writes to happen that may undo parts of
         # the factory reset, so ungracefully reboot to minimize the time
@@ -110,15 +110,36 @@ defmodule Nerves.Runtime.FwupOps do
 
     with {:ok, ops_fw} <- ops_fw_path(opts),
          {:ok, fwup} <- fwup_path(opts) do
-      params = [ops_fw, "-t", task, "-d", devpath, "-q", "-U", "--enable-trim"]
+      params = [
+        "-a",
+        "-i",
+        ops_fw,
+        "-t",
+        task,
+        "-d",
+        devpath,
+        "-q",
+        "-U",
+        "--enable-trim",
+        "--framing"
+      ]
 
       case System.cmd(fwup, params, cmd_opts) do
-        {_, 0} -> :ok
-        {result, _} -> {:error, result}
+        {results, 0} -> {:ok, results}
+        {result, _} -> output_to_error(result)
       end
     end
   end
 
+  defp output_to_error(raw_result) do
+    with {:ok, result} <- deframe(raw_result, []) do
+      Enum.find(result, {:error, "Unknown"}, &find_error/1)
+    end
+  end
+
+  defp find_error({:error, _message}), do: true
+  defp find_error(_status), do: false
+
   defp fwup_path(opts) do
     fwup_path =
       opts[:fwup_path] || Application.get_env(:nerves_runtime, :fwup_path, "fwup")
@@ -141,4 +162,28 @@ defmodule Nerves.Runtime.FwupOps do
       true -> {:error, "ops.fw or revert.fw not found in Nerves system"}
     end
   end
+
+  defp ignore_success_results({:ok, _}), do: :ok
+  defp ignore_success_results(other), do: other
+
+  defp deframe(<<length::32, payload::binary-size(length), rest::binary>>, acc) do
+    case decode(payload) do
+      {:ok, result} -> deframe(rest, [result | acc])
+      {:error, _} -> {:error, "Invalid framing"}
+    end
+  end
+
+  defp deframe(<<>>, acc) do
+    {:ok, Enum.reverse(acc)}
+  end
+
+  defp deframe(_, _acc) do
+    {:error, "Invalid framing"}
+  end
+
+  defp decode(<<"OK", _result::16, _meassage::binary>>), do: {:ok, :ok}
+  defp decode(<<"ER", _error_code::16, message::binary>>), do: {:ok, {:error, message}}
+  defp decode(<<"WN", _code::16, meassage::binary>>), do: {:ok, {:warning, meassage}}
+  defp decode(<<"PR", percent::16>>), do: {:ok, {:progress, percent}}
+  defp decode(_), do: {:error, "Invalid message"}
 end

mix.exs

@@ -42,7 +42,8 @@ defmodule Nerves.Runtime.MixProject do
       {:nerves_uevent, "~> 0.1.0"},
       {:ex_doc, "~> 0.22", only: :docs, runtime: false},
       {:dialyxir, "~> 1.1", only: :dev, runtime: false},
-      {:credo, "~> 1.5", only: :dev, runtime: false}
+      {:credo, "~> 1.5", only: :dev, runtime: false},
+      {:credo_binary_patterns, "~> 0.2.2", only: :dev, runtime: false}
     ]
   end
 

mix.lock

@@ -1,6 +1,7 @@
 %{
   "bunt": {:hex, :bunt, "1.0.0", "081c2c665f086849e6d57900292b3a161727ab40431219529f13c4ddcf3e7a44", [:mix], [], "hexpm", "dc5f86aa08a5f6fa6b8096f0735c4e76d54ae5c9fa2c143e5a1fc7c1cd9bb6b5"},
   "credo": {:hex, :credo, "1.7.10", "6e64fe59be8da5e30a1b96273b247b5cf1cc9e336b5fd66302a64b25749ad44d", [:mix], [{:bunt, "~> 0.2.1 or ~> 1.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:file_system, "~> 0.2 or ~> 1.0", [hex: :file_system, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "71fbc9a6b8be21d993deca85bf151df023a3097b01e09a2809d460348561d8cd"},
+  "credo_binary_patterns": {:hex, :credo_binary_patterns, "0.2.6", "cfcaca0bc5c6447b96c5a03eff175c28f86c486be8e95d55b360fb90c2dd18bd", [:mix], [{:credo, "~> 1.6", [hex: :credo, repo: "hexpm", optional: false]}], "hexpm", "d36a2b56ad72bdf3183ccc81d7e7821e78c97de7c127bc8dd99a5f05ca702187"},
   "dialyxir": {:hex, :dialyxir, "1.4.5", "ca1571ac18e0f88d4ab245f0b60fa31ff1b12cbae2b11bd25d207f865e8ae78a", [:mix], [{:erlex, ">= 0.2.7", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "b0fb08bb8107c750db5c0b324fa2df5ceaa0f9307690ee3c1f6ba5b9eb5d35c3"},
   "earmark_parser": {:hex, :earmark_parser, "1.4.41", "ab34711c9dc6212dda44fcd20ecb87ac3f3fce6f0ca2f28d4a00e4154f8cd599", [:mix], [], "hexpm", "a81a04c7e34b6617c2792e291b5a2e57ab316365c2644ddc553bb9ed863ebefa"},
   "elixir_make": {:hex, :elixir_make, "0.9.0", "6484b3cd8c0cee58f09f05ecaf1a140a8c97670671a6a0e7ab4dc326c3109726", [:mix], [], "hexpm", "db23d4fd8b757462ad02f8aa73431a426fe6671c80b200d9710caf3d1dd0ffdb"},

test/fixture/Makefile

@@ -0,0 +1,6 @@
+all: ops-fail.fw ops.fw
+
+%.fw: %.conf
+	fwup -c -f $< -o $@
+
+.PHONY: all

test/fixture/ops-fail.conf

@@ -0,0 +1,63 @@
+# Post-installation firmware operations (UNIT TEST VERSION)
+#
+# Run: fwup -c -f ops-fail.conf -o ops-fail.fw
+#
+# Tasks include:
+#
+# * `factory-reset` - Clear out the writable filesystem and any other writable
+#                   areas so that they can be re-initialized on the next boot.
+# * `prevent-revert` - Prevent `revert` from working until the next firmware
+# * `revert` - Revert to the previous firmware if it's still available
+# * `validate` - Mark this firmware as a good update.
+# * `status` - Print out which partition is active (`a` or `b`)
+#
+# To use:
+#
+# 1. Run `fwup -c -f fwup-ops.conf -o ops.fw` and copy ops.fw to
+#    the device. This is done automatically as part of the Nerves system
+#    build process. The file is stored in `/usr/share/fwup/ops.fw`.
+# 2. On the device, run `fwup -t <task> -d /dev/rootdisk0 --enable-trim /usr/share/fwup/ops.fw`.
+# 3. Reboot after running `revert` or `factory-reset`.
+
+require-fwup-version="1.0.0"
+
+##
+# factory-reset
+##
+task factory-reset {
+    on-init { error("factory-reset error") }
+}
+
+##
+# prevent-revert
+#
+# Pass `--enable-trim` to also clear out the partition that no longer should be used.
+##
+task prevent-revert {
+    on-init { error("prevent-revert error") }
+}
+
+##
+# revert
+##
+task revert {
+    on-init { error("revert error") }
+}
+
+##
+# status
+#
+# Run "fwup /usr/share/fwup/ops.fw -t status -d /dev/rootdisk0 -q -U" to check the status.
+##
+task status {
+    on-init { error("status error") }
+}
+
+##
+# validate
+#
+# The fwup configuration for this device always validates, so this doesn't do anything.
+##
+task validate {
+    on-init { error("validate error") }
+}

test/nerves_runtime/fwup_ops_test.exs

@@ -9,6 +9,8 @@ defmodule NervesRuntime.FwupOpsTest do
     reboot: false
   ]
 
+  @fwup_fail_options [ops_fw_path: Path.expand("test/fixture/ops-fail.fw")] ++ @fwup_options
+
   setup do
     # Even though this can be specified via an option, use the Application environment
     # since that's how it's normally set in practice.
@@ -23,21 +25,29 @@ defmodule NervesRuntime.FwupOpsTest do
   test "revert" do
     assert :ok = FwupOps.revert(@fwup_options)
     assert read_output() == "revert"
+
+    assert {:error, "revert error"} = FwupOps.revert(@fwup_fail_options)
   end
 
   test "factory reset" do
     assert :ok = FwupOps.factory_reset(@fwup_options)
     assert read_output() == "factory-reset"
+
+    assert {:error, "factory-reset error"} = FwupOps.factory_reset(@fwup_fail_options)
   end
 
   test "validate" do
     assert :ok = FwupOps.validate(@fwup_options)
     assert read_output() == "validate"
+
+    assert {:error, "validate error"} = FwupOps.validate(@fwup_fail_options)
   end
 
   test "prevent_revert" do
     assert :ok = FwupOps.prevent_revert(@fwup_options)
     assert read_output() == "prevent-revert"
+
+    assert {:error, "prevent-revert error"} = FwupOps.prevent_revert(@fwup_fail_options)
   end
 
   test "missing ops.fw" do