Small improvements to api.platforms.android.

appengine/swarming/swarming_bot/api/platforms/android.py

Index: appengine/swarming/swarming_bot/api/platforms/android.py
diff --git a/appengine/swarming/swarming_bot/api/platforms/android.py b/appengine/swarming/swarming_bot/api/platforms/android.py
index 00a810371e05c8022e51c90e73764f3717afa2ad..d0999382e7c9f26c11e4b1c41aebf25ded3656ec 100644
--- a/appengine/swarming/swarming_bot/api/platforms/android.py
+++ b/appengine/swarming/swarming_bot/api/platforms/android.py
@@ -14,8 +14,8 @@ import re
 import subprocess
 import sys
 
-# This file must be imported from swarming_bot.zip (or with '..' in sys.path).
-# libusb1 must have been put in the path already.
+# This file must be imported from swarming_bot.zip (or with '../..' in
+# sys.path). libusb1 must have been put in the path already.
 
 import adb
 import adb.adb_commands
@@ -51,9 +51,13 @@ pkcs1.HASH_METHODS['SHA-1-PREHASHED'] = _Accum
 pkcs1.HASH_ASN1['SHA-1-PREHASHED'] = pkcs1.HASH_ASN1['SHA-1']
 
 
-# Set when ADB is initialized. It contains one or multiple key used to
-# authenticate to Android debug protocol (adb).
+# Both following are set when ADB is initialized.
+# _ADB_KEYS is set to a list of PythonRSASigner instances. It contains one or
+# multiple key used to authenticate to Android debug protocol (adb).
 _ADB_KEYS = None
+# _ADB_KEYS_RAW is set to a dict(pub: priv) when initialized, with the raw
+# content of each key.
+_ADB_KEYS_RAW = None
 
 
 # Cache of /system/build.prop on Android devices connected to this host.
@@ -92,7 +96,7 @@ def _parcel_to_list(lines):
   return out
 
 
-class _Device(object):
+class Device(object):
   """Wraps an AdbCommands to make it exception safe.
 
   The fact that exceptions can be thrown any time makes the client code really
@@ -186,7 +190,7 @@ class _Device(object):
 
 
 def _load_device(handle):
-  """Given a adb.USBDevice, return a _Device wrapping an
+  """Given a adb.USBDevice, return a Device wrapping an
   adb_commands.AdbCommands.
   """
   port_path = '/'.join(map(str, handle.port_path))
@@ -239,9 +243,9 @@ def _load_device(handle):
       # is garbled.
       logging.warning(
           'Trying unpluging and pluging it back: %s: %s', port_path, e)
-  # Always create a _Device, even if it points to nothing. It makes using the
+  # Always create a Device, even if it points to nothing. It makes using the
   # client code much easier.
-  return _Device(cmd, port_path)
+  return Device(cmd, port_path)
 
 
 ### Public API.
@@ -254,32 +258,29 @@ def initialize(pub_key, priv_key):
   ~/.android/adbkey.pub.
   """
   global _ADB_KEYS
+  global _ADB_KEYS_RAW
+  if _ADB_KEYS is not None:
+    assert False, 'initialize() was called repeatedly: ignoring keys'
   assert bool(pub_key) == bool(priv_key)
-  if _ADB_KEYS is None:
-    _ADB_KEYS = []
-    if pub_key:
-      try:
-        _ADB_KEYS.append(PythonRSASigner(pub_key, priv_key))
-      except ValueError as exc:
-        logging.warning('Ignoring adb private key: %s', exc)
-
-    # Try to add local adb keys if available.
-    path = os.path.expanduser('~/.android/adbkey')
-    if os.path.isfile(path) and os.path.isfile(path+'.pub'):
-      with open(path + '.pub', 'rb') as f:
-        pub = f.read()
-      with open(path, 'rb') as f:
-        priv = f.read()
-      try:
-        _ADB_KEYS.append(PythonRSASigner(pub, priv))
-      except ValueError as exc:
-        logging.warning('Ignoring adb private key %s: %s', path, exc)
-
-    if not _ADB_KEYS:
-      return False
-  else:
-    if pub_key:
-      logging.warning('initialize() was called repeatedly: ignoring keys')
+  pub_key = pub_key.strip() if pub_key else pub_key
+  priv_key = priv_key.strip() if priv_key else priv_key
+
+  _ADB_KEYS = []
+  _ADB_KEYS_RAW = {}
+  if pub_key:
+    _ADB_KEYS.append(PythonRSASigner(pub_key, priv_key))
+    _ADB_KEYS_RAW[pub_key] = priv_key
+
+  # Try to add local adb keys if available.
+  path = os.path.expanduser('~/.android/adbkey')
+  if os.path.isfile(path) and os.path.isfile(path + '.pub'):
+    with open(path + '.pub', 'rb') as f:
+      pub = f.read().strip()
+    with open(path, 'rb') as f:
+      priv = f.read().strip()
+    _ADB_KEYS.append(PythonRSASigner(pub, priv))
+    _ADB_KEYS_RAW[pub] = priv
+
   return bool(_ADB_KEYS)
 
 
@@ -314,17 +315,33 @@ def load_rsa_private_key(pem):
 
 
 def kill_adb():
-  """adb sucks. Kill it with fire."""
+  """Stops the adb daemon.
+
+  The Swarming bot doesn't use the Android's SDK adb. It uses python-adb to
+  directly communicate with the devices. This works much better when a lot of
+  devices are connected to the host.
+
+  adb's stability is less than stellar. Kill it with fire.
+  """
   if not adb:
     return
-  try:
-    subprocess.call(['adb', 'kill-server'])
-  except OSError:
-    pass
-  subprocess.call(['killall', '--exact', 'adb'])
+  while True:
+    try:
+      subprocess.check_output(['pgrep', 'adb'])
+    except subprocess.CalledProcessError:
+      return
+    try:
+      subprocess.call(
+          ['adb', 'kill-server'],
+          stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    except OSError:
+      pass
+    subprocess.call(
+        ['killall', '--exact', 'adb'],
+        stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
 
 
-def get_devices():
+def get_devices(bot=None):  # pylint: disable=unused-argument

[ghost stip (do not use), 2015/10/08 20:13:28]

?

   """Returns the list of devices available.
 
   Caller MUST call close_devices(devices) on the return value.
@@ -371,16 +388,22 @@ def close_devices(devices):
 
 
 class CpuScalingGovernor(object):
-  """List of valid CPU scaling governor values."""
-  ON_DEMAND = 'ondemand'
+  """List of known CPU scaling governor values.
+  """
+  CONSERVATIVE = 'conservative'  # Not available on Nexus 10
+  INTERACTIVE = 'interactive'  # Default on Nexus 10
+  ON_DEMAND = 'ondemand'  # Not available on Nexus 10. Default on Nexus 4
   PERFORMANCE = 'performance'
-  CONSERVATIVE = 'conservative'
-  POWER_SAVE = 'powersave'
+  POWER_SAVE = 'powersave'  # Not available on Nexus 10
   USER_DEFINED = 'userspace'
 
   @classmethod
   def is_valid(cls, name):
-    return name in [getattr(cls, m) for m in dir(cls) if m[0].isupper()]
+    return name in cls.all_values()
+
+  @classmethod
+  def all_values(cls):

[ghost stip (do not use), 2015/10/08 20:13:28]

I don't know why you're doing all this. why not just use the string values
directly?

+    return sorted(getattr(cls, m) for m in dir(cls) if m[0].isupper())
 
 
 def set_cpu_scaling_governor(device, governor):
@@ -389,7 +412,7 @@ def set_cpu_scaling_governor(device, governor):
   Returns:
     True on success.
   """
-  assert isinstance(device, _Device), device
+  assert isinstance(device, Device), device
   assert CpuScalingGovernor.is_valid(governor), governor
   _, exit_code = device.shell(
       'echo "%s">/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor' %
@@ -403,7 +426,7 @@ def set_cpu_scaling(device, speed):
   Returns:
     True on success.
   """
-  assert isinstance(device, _Device), device
+  assert isinstance(device, Device), device
   assert isinstance(speed, int), speed
   assert 10000 <= speed <= 10000000, speed
   success = set_cpu_scaling_governor(device, CpuScalingGovernor.USER_DEFINED)
@@ -436,6 +459,7 @@ def get_build_prop(device):
 
 def get_temp(device):
   """Returns the device's 2 temperatures if available."""
+  assert isinstance(device, Device), device
   # Not all devices export this file. On other devices, the only real way to
   # read it is via Java
   # developer.android.com/guide/topics/sensors/sensors_environment.html
@@ -443,12 +467,16 @@ def get_temp(device):
   for i in xrange(2):
     out, _ = device.shell('cat /sys/class/thermal/thermal_zone%d/temp' % i)
     if out:
-      temps.append(int(out))
+      try:
+        temps.append(int(out))
+      except ValueError:
+        pass
   return temps
 
 
 def get_battery(device):
   """Returns details about the battery's state."""
+  assert isinstance(device, Device), device
   props = {}
   out = _dumpsys(device, 'battery')
   if not out:
@@ -457,12 +485,14 @@ def get_battery(device):
     if line.endswith(u':'):
       continue
     # On Android 4.1.2, it uses "voltage:123" instead of "voltage: 123".
-    key, value = line.split(u':', 2)
-    props[key.lstrip()] = value.strip()
+    parts = line.split(u':', 2)
+    if len(parts) == 2:
+      key, value = parts
+      props[key.lstrip()] = value.strip()
   out = {u'power': []}
-  if props[u'AC powered'] == u'true':
+  if props.get(u'AC powered') == u'true':
     out[u'power'].append(u'AC')
-  if props[u'USB powered'] == u'true':
+  if props.get(u'USB powered') == u'true':
     out[u'power'].append(u'USB')
   if props.get(u'Wireless powered') == u'true':
     out[u'power'].append(u'Wireless')
@@ -473,6 +503,7 @@ def get_battery(device):
 
 def get_cpu_scale(device):
   """Returns the CPU scaling factor."""
+  assert isinstance(device, Device), device
   mapping = {
     'cpuinfo_max_freq': u'max',
     'cpuinfo_min_freq': u'min',
@@ -487,6 +518,7 @@ def get_cpu_scale(device):
 
 def get_uptime(device):
   """Returns the device's uptime in second."""
+  assert isinstance(device, Device), device
   out, _ = device.shell('cat /proc/uptime')
   if out:
     return float(out.split()[1])
@@ -495,6 +527,7 @@ def get_uptime(device):
 
 def get_disk(device):
   """Returns details about the battery's state."""
+  assert isinstance(device, Device), device
   props = {}
   out = _dumpsys(device, 'diskstats')
   if not out:
@@ -502,13 +535,15 @@ def get_disk(device):
   for line in out.splitlines():
     if line.endswith(u':'):
       continue
-    key, value = line.split(u': ', 2)
-    match = re.match(u'^(\d+)K / (\d+)K.*', value)
-    if match:
-      props[key.lstrip()] = {
-        'free_mb': round(float(match.group(1)) / 1024., 1),
-        'size_mb': round(float(match.group(2)) / 1024., 1),
-      }
+    parts = line.split(u': ', 2)

[ghost stip (do not use), 2015/10/08 20:13:28]

I would have preferred this to be a separate CL to make it easier to review

+    if len(parts) == 2:
+      key, value = parts
+      match = re.match(u'^(\d+)K / (\d+)K.*', value)
+      if match:
+        props[key.lstrip()] = {
+          'free_mb': round(float(match.group(1)) / 1024., 1),
+          'size_mb': round(float(match.group(2)) / 1024., 1),
+        }
   return {
     u'cache': props[u'Cache-Free'],
     u'data': props[u'Data-Free'],
@@ -518,6 +553,7 @@ def get_disk(device):
 
 def get_imei(device):
   """Returns the phone's IMEI."""
+  assert isinstance(device, Device), device
   # Android <5.0.
   out = _dumpsys(device, 'iphonesubinfo')
   if out:
@@ -538,6 +574,7 @@ def get_imei(device):
 
 def get_ip(device):
   """Returns the IP address."""
+  assert isinstance(device, Device), device
   # If ever needed, read wifi.interface from /system/build.prop if a device
   # doesn't use wlan0.
   ip, _ = device.shell('getprop dhcp.wlan0.ipaddress')