Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent attempting to firmware update while another in progress #48

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Prevent attempting to firmware update while another in progress #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion lib/ssh_subsystem_fwup.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ defmodule SSHSubsystemFwup do
@impl :ssh_client_channel
def handle_msg({:ssh_channel_up, channel_id, cm}, state) do
with {:ok, options} <- precheck(state.options[:precheck_callback], state.options),
:ok <- check_devpath(options[:devpath]) do
:ok <- check_devpath(options[:devpath]),
:ok <- check_fwup_not_running() do
Logger.debug("ssh_subsystem_fwup: starting fwup")
fwup = FwupPort.open_port(options)
{:ok, %{state | id: channel_id, cm: cm, fwup: fwup}}
Expand Down Expand Up @@ -210,6 +211,14 @@ defmodule SSHSubsystemFwup do
end
end

defp check_fwup_not_running() do
if Enum.any?(Port.list(), &("#{Port.info(&1)[:name]}" =~ ~r/fwup/)) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is clever enough that I feel compelled to think if there's another way. But, I can definitely see how this works and prevents two instances of fwup running.

Fwiw, the best other idea I have at the moment would be for fwup to create a lock file based on the output when it runs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, I had the same thought, researched a bit, then decided to just start with the easiest

We could also just wrap this port process in a singleton genserver or something as well which would control fwup processes. I'm open to whatever.

It seems like checking for a running fwup port process is going to be just as effective as checking for a file on disk. The file seems like more risk to me in case the fwup process crashes without cleaning it up and thinks look to still be running (but I might guess you know the C way to get around that 😂)

{:error, "update already in progress"}
else
:ok
end
end

defp precheck(nil, options), do: {:ok, options}

defp precheck({m, f, a}, options) do
Expand Down
18 changes: 17 additions & 1 deletion test/ssh_subsystem_fwup_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ defmodule SSHSubsystemFwupTest do
def start_sshd(options) do
{:ok, ref} =
:ssh.daemon(@port, [
{:max_sessions, 1},
{:max_sessions, options[:max_sessions] || 1},
{:user_passwords, [{'user', 'password'}]},
{:system_dir, 'test/fixtures'},
{:subsystems, [SSHSubsystemFwup.subsystem_spec(options)]}
Expand Down Expand Up @@ -285,4 +285,20 @@ defmodule SSHSubsystemFwupTest do
# Check that the update was applied
assert match?(<<"Hello, world!", _::binary()>>, File.read!(options[:devpath]))
end

test "does not allow more than one update at a time", context do
options = default_options(context.test) |> Keyword.put(:max_sessions, 2)
File.touch!(options[:devpath])
start_sshd(options)

large_fw = Fwup.create_firmware(message: :binary.copy("a", 24 * 1024 * 1024))
fw_contents = Fwup.create_firmware()

{:ok, t} = Task.start(fn -> do_ssh(large_fw) end)
{output, exit_status} = do_ssh(fw_contents)

assert exit_status != 0
assert output =~ "Error: update already in progress"
Process.exit(t, :kill)
end
end