[Merge] lp:~mandel/ubuntu-location-provider-here/work-with-locked-sims into lp:ubuntu-location-provider-here
Loïc Minier
loic.minier at ubuntu.com
Wed Sep 3 12:50:34 UTC 2014
I'm a bit worried about various parts of this: sleep, /tmp, and the wait loop on modems.
It feels like instead we should start when the modem are ready, at the worst by patching ofono to emit new upstart events, at the best by listening to the right event (seems Android props would be the only way to get one though).
Diff comments:
> === modified file 'etc/init/ubuntu-location-provider-here-posclientd.conf'
> --- etc/init/ubuntu-location-provider-here-posclientd.conf 2014-09-01 20:24:15 +0000
> +++ etc/init/ubuntu-location-provider-here-posclientd.conf 2014-09-03 12:36:09 +0000
> @@ -7,31 +7,8 @@
> env BASE="/custom/vendor/here/location-provider"
> env STORAGE="/userdata/system-data/var/lib/ubuntu-location-provider-here"
>
> -# set the env vars to be empty so that they are always present and then provide
> -# the required data
> -
> -env HERE_SIM_MNC=""
> -export HERE_SIM_MNC
> -
> -env HERE_SIM_MCC=""
> -export HERE_SIM_MCC
> -
> -env HERE_UNIQUE_DEVICE_ID=""
> -export HERE_UNIQUE_DEVICE_ID
> -
> -env HERE_IMEI_FRAGMENT=""
> -export HERE_IMEI_FRAGMENT
> -
> -env HERE_DEVICE_MANUFACTURER=""
> -export HERE_DEVICE_MANUFACTURER
> -
> -env HERE_DEVICE_FIRMWARE_VERSION=""
> -export HERE_DEVICE_FIRMWARE_VERSION
> -
> -env HERE_DEVICE_TERMINAL_MODEL=""
> -export HERE_DEVICE_TERMINAL_MODEL
> -
> pre-start script
> + # only allow the daemon to run when the blobs are present and the user agreed to th license
(typo)
> if ! [ -x "$BASE/bin/@DEB_HOST_MULTIARCH@/posclientd" ]; then
> stop
> fi
> @@ -40,48 +17,96 @@
> if ! herepositioning-license-accepted; then
> stop
> fi
> +
> + # give some time to ofono, he is slow
> + sleep 5
sleep is pretty bad; this might be racy in other conditions (e.g. if it takes more than 5 seconds because load is high or whatever); what are we waiting for here? could we patch ofono to emit the right event instead?
> + # We support phones with more than one modem and therefore we need to know how many
> + # of them we have to check if we are online in each of them
> + DBUS_SEND="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono / org.ofono.Manager.GetModems || true)"
> + modems=$(echo "$DBUS_SEND" | grep -o /ril_[0-9]* | sort -V | tr "\n" " ")
> +
> + if [ "$modems" = "" ]; then
> + echo "ERROR: No modems have been found!";
> + exit 1;
Did you mean to stop rather than exit 1 here?
Also, it is important to distinguish no modem from dbus error?
(BTW you don't need the ; at the end of the lines)
> + fi
> +
> + MODEM_GET_PROPERTIES="org.ofono.Modem.GetProperties"
> + ONLINE_PROPERTY="Online"
> +
> + is_online=false
> + while ! $is_online; do
this really should be a string test:
while [ $is_online = false ]; do
or just while: since you break when is_online is set to true anyway
> + # get if one of the many modems is online
> + for modem in $modems; do
> + DBUS_SEND="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono $modem $MODEM_GET_PROPERTIES || true)"
> + dbus_reply="$(echo "$DBUS_SEND" | grep $ONLINE_PROPERTY | grep -o true || true)"
> + if [ "$dbus_reply" = "true" ]; then
> + ONLINE_MODEM="$modem"
> + echo "ONLINE_MODEM=$modem" >> "/tmp/$UPSTART_JOB"
> + is_online=true
> + break;
> + else
> + echo "Wait to be online"
> + sleep 1;
(extra ;)
> + fi
> + done
> + done
> +
> + # we do not need to get online to get the following information
> + DBUS_SEND="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono $ONLINE_MODEM org.ofono.Modem.GetProperties || true)"
> +
> + HERE_IMEI_FRAGMENT=$(echo "$DBUS_SEND" | grep Serial | grep -o '[0-9]*' | cut -c 1-8)
> + echo "HERE_IMEI_FRAGMENT=$HERE_IMEI_FRAGMENT" >> "/tmp/$UPSTART_JOB"
> + HERE_UNIQUE_DEVICE_ID="$(dbus-send --system --print-reply=literal --type=method_call --dest=com.ubuntu.WhoopsiePreferences /com/ubuntu/WhoopsiePreferences com.ubuntu.WhoopsiePreferences.GetIdentifier | sed -e 's/^[ \t]*//' || true)"
> + echo "HERE_UNIQUE_DEVICE_ID=$HERE_UNIQUE_DEVICE_ID" >> "/tmp/$UPSTART_JOB"
> + HERE_DEVICE_TERMINAL_MODEL="$(getprop ro.product.model)"
> + echo "HERE_DEVICE_TERMINAL_MODEL=$HERE_DEVICE_TERMINAL_MODEL" >> "/tmp/$UPSTART_JOB"
> + HERE_DEVICE_MANUFACTURER="$(getprop ro.product.manufacturer)"
> + echo "HERE_DEVICE_MANUFACTURER=$HERE_DEVICE_MANUFACTURER" >> "/tmp/$UPSTART_JOB"
> + HERE_DEVICE_FIRMWARE_VERSION="$(getprop ro.build.version.incremental)"
> + echo "HERE_DEVICE_FIRMWARE_VERSION=$HERE_DEVICE_FIRMWARE_VERSION" >> "/tmp/$UPSTART_JOB"
> +
> + unlocked="$(getprop gsm.sim.ril.phbready)"
> + if [ "$unlocked" = "true" ]; then
> + echo "Sim is unlocked"
> + is_online=false
> + while ! $is_online; do
> + # get the first modem with a sim manager interface
> + for modem in $modems; do
> + DBUS_SEND="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono $modem org.ofono.SimManager.GetProperties || true)"
> + if [ ! "$DBUS_SEND" = "" ]; then
> + is_online=true
> + HERE_SIM_MCC=$(echo "$DBUS_SEND" | grep MobileCountryCode | grep -o '[0-9]\{3\}')
> + echo "HERE_SIM_MCC=$HERE_SIM_MCC" >> "/tmp/$UPSTART_JOB"
> +
> + HERE_SIM_MNC=$(echo "$DBUS_SEND" | grep MobileNetworkCode | grep -o '[0-9]\{2\}')
> + echo "HERE_SIM_MNC=$HERE_SIM_MNC" >> "/tmp/$UPSTART_JOB"
> + fi
> + done
> + done
> +
> + if [ "$HERE_SIM_MCC" = "" ]; then
> + exit 1
> + fi
> +
> + if [ "$HERE_SIM_MNC" = "" ]; then
> + exit 1
> + fi
> + else
> + echo "Sim is locked"
> + echo "HERE_SIM_MNC=" >> "/tmp/$UPSTART_JOB"
> + echo "HERE_SIM_MCC=" >> "/tmp/$UPSTART_JOB"
> + fi
> +
> + mkdir -p "$STORAGE"
> end script
>
> script
> - # try to set the values needed by each of the env variables before we start
> - # the here processes
> -
> - DBUS_METHOD="org.ofono.NetworkRegistration.GetProperties"
> - DBUS_CMD="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono /ril_0 $DBUS_METHOD || true)"
> - CODES="MobileCountryCode MobileNetworkCode"
> -
> - for INFO in $CODES; do
> - VAL=$(echo "$DBUS_CMD"| grep $INFO| sed -e 's/\(^[\.0-9]*\)[^.0-9]*/\1/g' -e 's/ .*$//')
> - eval $INFO=$VAL
> - done
> -
> - # set the ofono env vars
> - export HERE_SIM_MCC=$MobileCountryCode
> - export HERE_SIM_MNC=$MobileNetworkCode
> -
> - DBUS_METHOD="org.ofono.Modem.GetProperties"
> - DBUS_CMD="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono /ril_0 $DBUS_METHOD)"
> - CODES="Serial"
> -
> - for INFO in $CODES; do
> - VAL=$(echo "$DBUS_CMD"| grep $INFO| sed -e 's/\(^[\.0-9]*\)[^.0-9]*/\1/g' -e 's/ .*$//' | awk '{print substr($0,1,8)}')
> - eval $INFO=$VAL
> - done
> -
> - export HERE_IMEI_FRAGMENT=$Serial
> -
> - # set the unique identifier
> - UNIQUE_ID=$(sudo gdbus call -y -d com.ubuntu.WhoopsiePreferences -o /com/ubuntu/WhoopsiePreferences -m com.ubuntu.WhoopsiePreferences.GetIdentifier | sed "s/['(),]//g")
> - export HERE_UNIQUE_DEVICE_ID=$UNIQUE_ID
> -
> - export HERE_DEVICE_TERMINAL_MODEL="$(getprop ro.product.model)"
> - export HERE_DEVICE_MANUFACTURER="$(getprop ro.product.manufacturer)"
> - export HERE_DEVICE_FIRMWARE_VERSION="$(getprop ro.build.version.incremental)"
> -
> - mkdir -p "$STORAGE"
> -
> + . "/tmp/$UPSTART_JOB"
Is this to share setting the vars between scripts? I believe if you declare the env vars in the job (env XYZ) then you may set them in pre-start to be used in start, no?
> LD_LIBRARY_PATH="$BASE/lib/@DEB_HOST_MULTIARCH@"
> export LD_LIBRARY_PATH
> exec "$BASE/bin/@DEB_HOST_MULTIARCH@/posclientd" --preinst-dir "$BASE/share" --storage-dir "$STORAGE"
> +end script
>
> +post-start script
> + rm -f "/tmp/$UPSTART_JOB"
> end script
>
> === added file 'etc/init/ubuntu-location-provider-here-sim-watcher.conf'
> --- etc/init/ubuntu-location-provider-here-sim-watcher.conf 1970-01-01 00:00:00 +0000
> +++ etc/init/ubuntu-location-provider-here-sim-watcher.conf 2014-09-03 12:36:09 +0000
> @@ -0,0 +1,6 @@
> +start on android-container gsm.sim.ril.phbready=true
> +
> +script
> + respawn ubuntu-location-provider-here-posclientd
Hmm does respawn really exist?! can't find it here
> + respawn ubuntu-location-provider-here-slpgwd
> +end script
>
> === modified file 'etc/init/ubuntu-location-provider-here-slpgwd.conf'
> --- etc/init/ubuntu-location-provider-here-slpgwd.conf 2014-09-01 20:24:15 +0000
> +++ etc/init/ubuntu-location-provider-here-slpgwd.conf 2014-09-03 12:36:09 +0000
> @@ -7,30 +7,6 @@
> env BASE="/custom/vendor/here/location-provider"
> env STORAGE=/userdata/system-data/var/lib/ubuntu-location-provider-here
>
> -# set the env vars to be empty so that they are always present and then provide
> -# the required data
> -
> -env HERE_SIM_MNC=""
> -export HERE_SIM_MNC
> -
> -env HERE_SIM_MCC=""
> -export HERE_SIM_MCC
> -
> -env HERE_UNIQUE_DEVICE_ID=""
> -export HERE_UNIQUE_DEVICE_ID
> -
> -env HERE_IMEI_FRAGMENT=""
> -export HERE_IMEI_FRAGMENT
> -
> -env HERE_DEVICE_MANUFACTURER=""
> -export HERE_DEVICE_MANUFACTURER
> -
> -env HERE_DEVICE_FIRMWARE_VERSION=""
> -export HERE_DEVICE_FIRMWARE_VERSION
> -
> -env HERE_DEVICE_TERMINAL_MODEL=""
> -export HERE_DEVICE_TERMINAL_MODEL
> -
> pre-start script
> if ! [ -x "$BASE/bin/@DEB_HOST_MULTIARCH@/slpgwd" ]; then
> stop
> @@ -40,48 +16,96 @@
> if ! herepositioning-license-accepted; then
> stop
> fi
> +
> + # give some time to ofono, he is slow
> + sleep 5
> + # We support phones with more than one modem and therefore we need to know how many
> + # of them we have to check if we are online in each of them
> + DBUS_SEND="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono / org.ofono.Manager.GetModems || true)"
> + modems=$(echo "$DBUS_SEND" | grep -o /ril_[0-9]* | sort -V | tr "\n" " ")
> +
> + if [ "$modems" = "" ]; then
> + echo "ERROR: No modems have been found!";
> + exit 1;
> + fi
> +
> + MODEM_GET_PROPERTIES="org.ofono.Modem.GetProperties"
> + ONLINE_PROPERTY="Online"
> +
> + is_online=false
> + while ! $is_online; do
> + # get if one of the many modems is online
> + for modem in $modems; do
> + DBUS_SEND="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono $modem $MODEM_GET_PROPERTIES || true)"
> + dbus_reply="$(echo "$DBUS_SEND" | grep $ONLINE_PROPERTY | grep -o true || true)"
> + if [ "$dbus_reply" = "true" ]; then
> + ONLINE_MODEM="$modem"
> + echo "ONLINE_MODEM=$modem" >> "/tmp/$UPSTART_JOB"
> + is_online=true
> + break;
> + else
> + echo "Wait to be online"
> + sleep 1;
> + fi
> + done
> + done
> +
> + # we do not need to get online to get the following information
> + DBUS_SEND="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono $ONLINE_MODEM org.ofono.Modem.GetProperties || true)"
> +
> + HERE_IMEI_FRAGMENT=$(echo "$DBUS_SEND" | grep Serial | grep -o '[0-9]*' | cut -c 1-8)
> + echo "HERE_IMEI_FRAGMENT=$HERE_IMEI_FRAGMENT" >> "/tmp/$UPSTART_JOB"
> + HERE_UNIQUE_DEVICE_ID="$(dbus-send --system --print-reply=literal --type=method_call --dest=com.ubuntu.WhoopsiePreferences /com/ubuntu/WhoopsiePreferences com.ubuntu.WhoopsiePreferences.GetIdentifier | sed -e 's/^[ \t]*//' || true)"
> + echo "HERE_UNIQUE_DEVICE_ID=$HERE_UNIQUE_DEVICE_ID" >> "/tmp/$UPSTART_JOB"
> + HERE_DEVICE_TERMINAL_MODEL="$(getprop ro.product.model)"
> + echo "HERE_DEVICE_TERMINAL_MODEL=$HERE_DEVICE_TERMINAL_MODEL" >> "/tmp/$UPSTART_JOB"
> + HERE_DEVICE_MANUFACTURER="$(getprop ro.product.manufacturer)"
> + echo "HERE_DEVICE_MANUFACTURER=$HERE_DEVICE_MANUFACTURER" >> "/tmp/$UPSTART_JOB"
> + HERE_DEVICE_FIRMWARE_VERSION="$(getprop ro.build.version.incremental)"
> + echo "HERE_DEVICE_FIRMWARE_VERSION=$HERE_DEVICE_FIRMWARE_VERSION" >> "/tmp/$UPSTART_JOB"
> +
> + unlocked="$(getprop gsm.sim.ril.phbready)"
> + if [ "$unlocked" = "true" ]; then
> + echo "Sim is unlocked"
> + is_online=false
> + while ! $is_online; do
> + # get the first modem with a sim manager interface
> + for modem in $modems; do
> + DBUS_SEND="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono $modem org.ofono.SimManager.GetProperties || true)"
> + if [ ! "$DBUS_SEND" = "" ]; then
> + is_online=true
> + HERE_SIM_MCC=$(echo "$DBUS_SEND" | grep MobileCountryCode | grep -o '[0-9]\{3\}')
> + echo "HERE_SIM_MCC=$HERE_SIM_MCC" >> "/tmp/$UPSTART_JOB"
> +
> + HERE_SIM_MNC=$(echo "$DBUS_SEND" | grep MobileNetworkCode | grep -o '[0-9]\{2\}')
> + echo "HERE_SIM_MNC=$HERE_SIM_MNC" >> "/tmp/$UPSTART_JOB"
> + fi
> + done
> + done
> +
> + if [ "$HERE_SIM_MCC" = "" ]; then
> + exit 1
> + fi
> +
> + if [ "$HERE_SIM_MNC" = "" ]; then
> + exit 1
> + fi
> + else
> + echo "Sim is locked"
> + echo "HERE_SIM_MNC=" >> "/tmp/$UPSTART_JOB"
> + echo "HERE_SIM_MCC=" >> "/tmp/$UPSTART_JOB"
> + fi
> +
> + mkdir -p "$STORAGE"
> end script
>
> script
> - # try to set the values needed by each of the env variables before we start
> - # the here processes
> -
> - DBUS_METHOD="org.ofono.NetworkRegistration.GetProperties"
> - DBUS_CMD="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono /ril_0 $DBUS_METHOD || true)"
> - CODES="MobileCountryCode MobileNetworkCode"
> -
> - for INFO in $CODES; do
> - VAL=$(echo "$DBUS_CMD"| grep $INFO| sed -e 's/\(^[\.0-9]*\)[^.0-9]*/\1/g' -e 's/ .*$//')
> - eval $INFO=$VAL
> - done
> -
> - # set the ofono env vars
> - export HERE_SIM_MCC=$MobileCountryCode
> - export HERE_SIM_MNC=$MobileNetworkCode
> -
> - DBUS_METHOD="org.ofono.Modem.GetProperties"
> - DBUS_CMD="$(dbus-send --system --print-reply=literal --type=method_call --dest=org.ofono /ril_0 $DBUS_METHOD)"
> - CODES="Serial"
> -
> - for INFO in $CODES; do
> - VAL=$(echo "$DBUS_CMD"| grep $INFO| sed -e 's/\(^[\.0-9]*\)[^.0-9]*/\1/g' -e 's/ .*$//' | awk '{print substr($0,1,8)}')
> - eval $INFO=$VAL
> - done
> -
> - export HERE_IMEI_FRAGMENT=$Serial
> -
> - # set the unique identifier
> - UNIQUE_ID=$(sudo gdbus call -y -d com.ubuntu.WhoopsiePreferences -o /com/ubuntu/WhoopsiePreferences -m com.ubuntu.WhoopsiePreferences.GetIdentifier | sed "s/['(),]//g")
> - export HERE_UNIQUE_DEVICE_ID=$UNIQUE_ID
> -
> - export HERE_DEVICE_TERMINAL_MODEL="$(getprop ro.product.model)"
> - export HERE_DEVICE_MANUFACTURER="$(getprop ro.product.manufacturer)"
> - export HERE_DEVICE_FIRMWARE_VERSION="$(getprop ro.build.version.incremental)"
> -
> - mkdir -p "$STORAGE"
> -
> + . "/tmp/$UPSTART_JOB"
> LD_LIBRARY_PATH="$BASE/lib/@DEB_HOST_MULTIARCH@"
> export LD_LIBRARY_PATH
> exec "$BASE/bin/@DEB_HOST_MULTIARCH@/slpgwd" --preinst-dir "$BASE/share" --storage-dir "$STORAGE"
> +end script
>
> +post-start script
> + rm -f "/tmp/$UPSTART_JOB"
> end script
>
--
https://code.launchpad.net/~mandel/ubuntu-location-provider-here/work-with-locked-sims/+merge/233204
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~mandel/ubuntu-location-provider-here/work-with-locked-sims into lp:ubuntu-location-provider-here.
More information about the Ubuntu-reviews
mailing list