Fix crash when trying to connect w/o an ICCID

If there are problems reading the ICCID off the SIM card, the APN
selection code repeatedly raises an exception.

This isn't good. It would be much better to log an error and raise an
alarm in a more general way.

This updates the code to validate the ICCID before trying to select an
APN. The code in the `with` was really hard to read, so this modifies
function calls to make it easier. The modifications are not intended to
change any semantics.

lib/vintage_net_qmi/connection.ex

@@ -43,14 +43,17 @@ defmodule VintageNetQMI.Connection do
     GenServer.cast(name(ifname), {:process_stats, stats})
   end
 
+  defp mobile_prop(ifname, key), do: ["interface", ifname, "mobile", key]
+
   @impl GenServer
   def init(args) do
     ifname = Keyword.fetch!(args, :ifname)
     providers = Keyword.fetch!(args, :service_providers)
     radio_technologies = Keyword.get(args, :radio_technologies)
 
-    VintageNet.subscribe(["interface", ifname, "mobile", "iccid"])
-    iccid = VintageNet.get(["interface", ifname, "mobile", "iccid"])
+    iccid_property = mobile_prop(ifname, "iccid")
+    VintageNet.subscribe(iccid_property)
+    iccid = VintageNet.get(iccid_property)
 
     state =
       %{
@@ -104,11 +107,7 @@ defmodule VintageNetQMI.Connection do
     timestamp = System.monotonic_time()
     stats_with_timestamp = Map.put(stats, :timestamp, timestamp)
 
-    PropertyTable.put(
-      VintageNet,
-      ["interface", state.ifname, "mobile", "statistics"],
-      stats_with_timestamp
-    )
+    PropertyTable.put(VintageNet, mobile_prop(state.ifname, "statistics"), stats_with_timestamp)
 
     {:noreply, state}
   end
@@ -140,14 +139,12 @@ defmodule VintageNetQMI.Connection do
 
   defp try_to_connect(state) do
     three_3gpp_profile_index = 1
+    iccid = state.iccid
+    providers = state.service_providers
 
-    with %{} = provider <-
-           ServiceProvider.select_provider_by_iccid(state.service_providers, state.iccid),
-         PropertyTable.put(
-           VintageNet,
-           ["interface", state.ifname, "mobile", "apn"],
-           provider.apn
-         ),
+    with :ok <- validate_iccid(iccid),
+         {:ok, provider} <- ServiceProvider.select_provider_by_iccid(providers, iccid),
+         PropertyTable.put(VintageNet, mobile_prop(state.ifname, "apn"), provider.apn),
          :ok <- set_roaming_allowed_for_provider(provider, three_3gpp_profile_index, state),
          {:ok, _} <-
            WirelessData.start_network_interface(state.qmi,
@@ -157,7 +154,7 @@ defmodule VintageNetQMI.Connection do
       Logger.info("[VintageNetQMI]: network started. Waiting on DHCP")
       state
     else
-      nil ->
+      {:error, :no_provider} ->
         Logger.warning(
           "[VintageNetQMI]: cannot select an APN to use from the configured service providers, check your configuration for VintageNet."
         )
@@ -178,6 +175,9 @@ defmodule VintageNetQMI.Connection do
     end
   end
 
+  defp validate_iccid(iccid) when is_binary(iccid), do: :ok
+  defp validate_iccid(_iccid), do: {:error, :missing_iccid}
+
   defp set_roaming_allowed_for_provider(
          %{roaming_allowed?: roaming_allowed?},
          profile_index,

lib/vintage_net_qmi/service_provider.ex

@@ -22,18 +22,18 @@ defmodule VintageNetQMI.ServiceProvider do
   @doc """
   Select the provider by the iccid
   """
-  @spec select_provider_by_iccid([t()], binary()) :: t() | nil
+  @spec select_provider_by_iccid([t()], binary()) :: {:ok, t()} | {:error, :no_provider}
   def select_provider_by_iccid(providers, iccid) do
-    Enum.reduce_while(providers, nil, fn
-      %{only_iccid_prefixes: prefixes} = provider, default ->
+    Enum.reduce_while(providers, {:error, :no_provider}, fn
+      %{only_iccid_prefixes: prefixes} = provider, best ->
         if is_binary(iccid) && String.starts_with?(iccid, prefixes) do
-          {:halt, provider}
+          {:halt, {:ok, provider}}
         else
-          {:cont, default}
+          {:cont, best}
         end
 
       provider, _default ->
-        {:cont, provider}
+        {:cont, {:ok, provider}}
     end)
   end
 end

test/vintage_net_qmi/service_provider_test.exs

@@ -8,7 +8,7 @@ defmodule VintageNetQMI.ServiceProviderTest do
       provider = %{apn: "fake"}
       iccid = "891004234814455936F"
 
-      assert provider == ServiceProvider.select_provider_by_iccid([provider], iccid)
+      assert {:ok, provider} == ServiceProvider.select_provider_by_iccid([provider], iccid)
     end
 
     test "when prefix option matches the ICCID prefixes" do
@@ -22,15 +22,18 @@ defmodule VintageNetQMI.ServiceProviderTest do
 
       [first_provider, second_provider | _] = providers
 
-      assert first_provider == ServiceProvider.select_provider_by_iccid(providers, first_iccid)
-      assert second_provider == ServiceProvider.select_provider_by_iccid(providers, second_iccid)
+      assert {:ok, first_provider} ==
+               ServiceProvider.select_provider_by_iccid(providers, first_iccid)
+
+      assert {:ok, second_provider} ==
+               ServiceProvider.select_provider_by_iccid(providers, second_iccid)
     end
 
     test "when prefix option does not match ICCID prefix" do
       provider = %{apn: "not me", only_iccid_prefixes: ["89171717"]}
       iccid = "891004234814455936F"
 
-      assert nil ==
+      assert {:error, :no_provider} ==
                ServiceProvider.select_provider_by_iccid([provider], iccid)
     end
 
@@ -44,7 +47,7 @@ defmodule VintageNetQMI.ServiceProviderTest do
 
       [_, this_one | _] = providers
 
-      assert this_one == ServiceProvider.select_provider_by_iccid(providers, iccid)
+      assert {:ok, this_one} == ServiceProvider.select_provider_by_iccid(providers, iccid)
     end
 
     test "default to service provider with out ICCID selection when non match" do
@@ -55,7 +58,8 @@ defmodule VintageNetQMI.ServiceProviderTest do
 
       iccid = "8947571711111111FF"
 
-      assert List.first(providers) == ServiceProvider.select_provider_by_iccid(providers, iccid)
+      assert {:ok, List.first(providers)} ==
+               ServiceProvider.select_provider_by_iccid(providers, iccid)
     end
 
     test "when iccid is nil select default without ICCID" do
@@ -66,7 +70,8 @@ defmodule VintageNetQMI.ServiceProviderTest do
 
       iccid = nil
 
-      assert List.first(providers) == ServiceProvider.select_provider_by_iccid(providers, iccid)
+      assert {:ok, List.first(providers)} ==
+               ServiceProvider.select_provider_by_iccid(providers, iccid)
     end
   end
 end