[Android] Tune DeviceUtils commands that are prone to large outputs.

build/android/pylib/device/device_utils.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Provides a variety of device interactions based on adb.
 
 Eventually, this will be based on adb_wrapper.
 """
 # pylint: disable=unused-argument
 
@@ skipping 554 matching lines @@
       if not output:
         return ''
       elif len(output) == 1:
         return output[0]
       else:
         msg = 'one line of output was expected, but got: %s'
         raise device_errors.CommandFailedError(msg % output, str(self))
     else:
       return output
 
+  def _RunPipedShellCommand(self, script, **kwargs):
+    PIPESTATUS_LEADER = 'PIPESTATUS: '
+
+    if not isinstance(script, basestring):
+      script = ' '.join(script)

[perezju, 2015/04/14 10:17:29]

I'm not sure I link this. The caller should be responsible for building the
script as needed.

[jbudorick, 2015/04/14 13:42:46]

I wasn't sure about this. Removed.

+
+    script += '; echo "%s${PIPESTATUS[@]}"' % PIPESTATUS_LEADER
+    output = self.RunShellCommand(script, **kwargs)
+    pipestatus_line = output[-1]
+    output = output[:-1]
+
+    if not pipestatus_line.startswith(PIPESTATUS_LEADER):
+      logging.error('exit statuses of shell script %r missing.', script)

[perezju, 2015/04/14 10:17:29]

RunShellCommand should have already logged the script as it was run. So here
maybe the message can just say something like: 'pipe exit statuses of shell
script missing.' ?

[jbudorick, 2015/04/14 13:42:46]

Done.

+      raise device_errors.AdbShellCommandFailedError(
+          script, output, status=None,

[perezju, 2015/04/14 10:17:29]

if the PIPESTATUS_LEADER is missing, maybe we want to add back the
|pipstatus_line| back to the |output| for the exception message?

[jbudorick, 2015/04/14 13:42:46]

moved output = output[:-1] below this check

+          device_serial=self.adb.GetDeviceSerial())
+    statuses = [
+        int(s) for s in pipestatus_line[len(PIPESTATUS_LEADER):].split()]
+    if any(statuses):
+      raise device_errors.AdbShellCommandFailedError(
+          script, output, status=statuses,
+          device_serial=self.adb.GetDeviceSerial())
+    return output
+
   @decorators.WithTimeoutAndRetriesFromInstance()
   def KillAll(self, process_name, signum=9, as_root=False, blocking=False,
               timeout=None, retries=None):
     """Kill all processes with the given name on the device.
 
     Args:
       process_name: A string containing the name of the process to kill.
       signum: An integer containing the signal number to send to kill. Defaults
               to 9 (SIGKILL).
       as_root: A boolean indicating whether the kill should be executed with
                root privileges.
       blocking: A boolean indicating whether we should wait until all processes
                 with the given |process_name| are dead.
       timeout: timeout in seconds
       retries: number of retries
 
     Raises:
       CommandFailedError if no process was killed.
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    pids = self._GetPidsImpl(process_name)
+    pids = self.GetPids(process_name)
     if not pids:
       raise device_errors.CommandFailedError(
           'No process "%s"' % process_name, str(self))
 
     cmd = ['kill', '-%d' % signum] + pids.values()
     self.RunShellCommand(cmd, as_root=as_root, check_return=True)
 
     if blocking:
       wait_period = 0.1
-      while self._GetPidsImpl(process_name):
+      while self.GetPids(process_name):
         time.sleep(wait_period)
 
     return len(pids)
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def StartActivity(self, intent_obj, blocking=False, trace_file_name=None,
                     force_stop=False, timeout=None, retries=None):
     """Start package's activity on the device.
 
     Args:
@@ skipping 394 matching lines @@
     ls_out = self.RunShellCommand(['ls', '-l', device_path], as_root=as_root,
                                   check_return=True)
     for line in ls_out:
       m = self._LS_RE.match(line)
       if m and m.group('name') == posixpath.basename(device_path):
         size = int(m.group('size'))
         break
     else:
       logging.warning('Could not determine size of %s.', device_path)
 
-    if size is None or size <= self._MAX_ADB_OUTPUT_LENGTH:
+    if size > 0 and size <= self._MAX_ADB_OUTPUT_LENGTH:

[perezju, 2015/04/14 10:17:29]

I think you can write this as: 0 < size <= self._MAX_ADB_OUTPUT_LENGTH

[jbudorick, 2015/04/14 13:42:46]

this is sorcery

done

       return _JoinLines(self.RunShellCommand(
           ['cat', device_path], as_root=as_root, check_return=True))
     elif as_root and self.NeedsSU():
       with device_temp_file.DeviceTempFile(self.adb) as device_temp:
         self.RunShellCommand(['cp', device_path, device_temp.name],
                              as_root=True, check_return=True)
         return self._ReadFileWithPull(device_temp.name)
     else:
       return self._ReadFileWithPull(device_path)
 
@@ skipping 314 matching lines @@
       retries: number of retries
 
     Returns:
       A dict mapping process name to PID for each process that contained the
       provided |process_name|.
 
     Raises:
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    return self._GetPidsImpl(process_name)
+    procs_pids = {}
+    try:
+      ps_output = self._RunPipedShellCommand(
+          'ps | grep -F %s' % process_name, check_return=True)

[perezju, 2015/04/14 10:17:29]

use cmd_helper.SingleQuote(process_name)

[jbudorick, 2015/04/14 13:42:46]

Done.

+    except device_errors.AdbShellCommandFailedError as e:

[perezju, 2015/04/14 10:17:29]

just to clarify, what is the relationship between the "pipe exit statuses" and
the regular exit status checked by check_return=True? 

In particular, are we sure that e.stats below will always be a list? Is it
possible that the pipe exit statues are all fine, but the "main" exit status
fails? (If not, maybe _RunPopedShellCommand should always run with
check_return=False and avoid the double check?)

[jbudorick, 2015/04/14 13:42:46]

I'm not sure whether $? is the same as the last item in $PIPESTATUS or if it's
any failure that happened in the pipe.

I suppose that, in that scenario, e.status would not be a list. I'm ok with
having check_return explicitly off in calls to _RunPipedShellCommand.

+      if e.status and not e.status[0]:
+        # If ps succeeded but grep failed, there were no processes with the
+        # given name.
+        return procs_pids
+      else:
+        raise
 
-  def _GetPidsImpl(self, process_name):
-    procs_pids = {}
-    for line in self.RunShellCommand('ps', check_return=True):
+    for line in ps_output:
       try:
         ps_data = line.split()
         if process_name in ps_data[-1]:
           procs_pids[ps_data[-1]] = ps_data[1]
       except IndexError:
         pass
     return procs_pids
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def TakeScreenshot(self, host_path=None, timeout=None, retries=None):
@@ skipping 51 matching lines @@
     except device_errors.CommandFailedError:
       logging.exception('Error getting memory usage from status')
 
     return result
 
   def _GetMemoryUsageForPidFromSmaps(self, pid):
     SMAPS_COLUMNS = (
         'Size', 'Rss', 'Pss', 'Shared_Clean', 'Shared_Dirty', 'Private_Clean',
         'Private_Dirty')
 
-    showmap_out = self.RunShellCommand(
-        ['showmap', str(pid)], as_root=True, check_return=True)
-    if not showmap_out:
-      raise device_errors.CommandFailedError('No output from showmap')
+    showmap_out = self._RunPipedShellCommand(
+        'showmap %s | grep TOTAL' % str(pid), as_root=True, check_return=True)

[perezju, 2015/04/14 10:17:29]

maybe rewrite as: 'showmap %d | grep TOTAL' % pid

so we don't need any quoting, while making sure that the pid is really a number.

[perezju, 2015/04/14 10:49:19]

actually, maybe: 'showmap %d | grep TOTAL' % int(pid)

because (I don't like it) but pids are often passed around as strings.

[jbudorick, 2015/04/14 13:42:46]

Done.

 
     split_totals = showmap_out[-1].split()
     if (not split_totals
         or len(split_totals) != 9
         or split_totals[-1] != 'TOTAL'):
       raise device_errors.CommandFailedError(
           'Invalid output from showmap: %s' % '\n'.join(showmap_out))
 
     return dict(itertools.izip(SMAPS_COLUMNS, (int(n) for n in split_totals)))
 
@@ skipping 85 matching lines @@
     """Returns client cache."""
     if client_name not in self._client_caches:
       self._client_caches[client_name] = {}
     return self._client_caches[client_name]
 
   def _ClearCache(self):
     """Clears all caches."""
     for client in self._client_caches:
       self._client_caches[client].clear()
     self._cache.clear()