From 486e00e23abaea4e71df7c429924ae0c88145aa8 Mon Sep 17 00:00:00 2001 From: perezju Date: Mon, 26 Jan 2015 02:10:00 -0800 Subject: [PATCH] Reland of Migrate DeviceUtils.ReadFile to adb_wrapper (try 3) Original description: The implementation is based on a simple 'cat' run by shell on the device (with optional as_root). Interface changes: - The return value is a string (instead of a list of lines). - An exception is raised if the file cannot be read (instead of returning an empty list). Previous CLs: - https://codereview.chromium.org/794583004/ - https://codereview.chromium.org/775333002/ BUG=267773 Review URL: https://codereview.chromium.org/806843002 Cr-Original-Commit-Position: refs/heads/master@{#313055} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 7138148b7108a41bf44a39e8aa52e5954d2c0f52 --- android/pylib/device/device_utils.py | 27 +++++----- android/pylib/device/device_utils_test.py | 57 ++++++-------------- android/pylib/flag_changer.py | 13 +++-- android/pylib/instrumentation/test_runner.py | 6 +-- android/pylib/perf/thermal_throttle.py | 2 +- android/tombstones.py | 3 +- 6 files changed, 45 insertions(+), 63 deletions(-) diff --git a/android/pylib/device/device_utils.py b/android/pylib/device/device_utils.py index a4ddef132..fab3c5d39 100644 --- a/android/pylib/device/device_utils.py +++ b/android/pylib/device/device_utils.py @@ -91,6 +91,13 @@ def _GetTimeStamp(): return time.strftime('%Y%m%dT%H%M%S', time.localtime()) +def _JoinLines(lines): + # makes sure that the last line is also terminated, and is more memory + # efficient than first appending an end-line to each line and then joining + # all of them together. + return ''.join(s for line in lines for s in (line, '\n')) + + class DeviceUtils(object): _VALID_SHELL_VARIABLE = re.compile('^[a-zA-Z_][a-zA-Z0-9_]*$') @@ -883,7 +890,8 @@ class DeviceUtils(object): self.adb.Pull(device_path, host_path) @decorators.WithTimeoutAndRetriesFromInstance() - def ReadFile(self, device_path, as_root=False, timeout=None, retries=None): + def ReadFile(self, device_path, as_root=False, + timeout=None, retries=None): """Reads the contents of a file from the device. Args: @@ -895,22 +903,17 @@ class DeviceUtils(object): retries: number of retries Returns: - The contents of the file at |device_path| as a list of lines. + The contents of |device_path| as a string. Contents are intepreted using + universal newlines, so the caller will see them encoded as '\n'. Also, + all lines will be terminated. Raises: - CommandFailedError if the file can't be read. + AdbCommandFailedError if the file can't be read. CommandTimeoutError on timeout. DeviceUnreachableError on missing device. """ - # TODO(jbudorick) Evaluate whether we want to return a list of lines after - # the implementation switch, and if file not found should raise exception. - if as_root: - if not self.old_interface.CanAccessProtectedFileContents(): - raise device_errors.CommandFailedError( - 'Cannot read from %s with root privileges.' % device_path) - return self.old_interface.GetProtectedFileContents(device_path) - else: - return self.old_interface.GetFileContents(device_path) + return _JoinLines(self.RunShellCommand( + ['cat', device_path], as_root=as_root, check_return=True)) @decorators.WithTimeoutAndRetriesFromInstance() def WriteFile(self, device_path, contents, as_root=False, force_push=False, diff --git a/android/pylib/device/device_utils_test.py b/android/pylib/device/device_utils_test.py index 72ab99ca0..e346beeac 100755 --- a/android/pylib/device/device_utils_test.py +++ b/android/pylib/device/device_utils_test.py @@ -1065,56 +1065,33 @@ class DeviceUtilsPullFileTest(DeviceUtilsNewImplTest): '/test/file/host/path') -class DeviceUtilsReadFileTest(DeviceUtilsOldImplTest): +class DeviceUtilsReadFileTest(DeviceUtilsNewImplTest): def testReadFile_exists(self): - with self.assertCallsSequence([ - ("adb -s 0123456789abcdef shell " - "'cat \"/read/this/test/file\" 2>/dev/null'", - 'this is a test file')]): - self.assertEqual(['this is a test file'], + with self.assertCall( + self.call.adb.Shell('cat /read/this/test/file'), + 'this is a test file\r\n'): + self.assertEqual('this is a test file\n', self.device.ReadFile('/read/this/test/file')) def testReadFile_doesNotExist(self): + with self.assertCall( + self.call.adb.Shell('cat /this/file/does.not.exist'), + self.ShellError('/system/bin/sh: cat: /this/file/does.not.exist: ' + 'No such file or directory')): + with self.assertRaises(device_errors.AdbCommandFailedError): + self.device.ReadFile('/this/file/does.not.exist') + + def testReadFile_withSU(self): with self.assertCalls( - "adb -s 0123456789abcdef shell " - "'cat \"/this/file/does.not.exist\" 2>/dev/null'", - ''): - self.device.ReadFile('/this/file/does.not.exist') - - def testReadFile_asRoot_withRoot(self): - self.device.old_interface._privileged_command_runner = ( - self.device.old_interface.RunShellCommand) - self.device.old_interface._protected_file_access_method_initialized = True - with self.assertCallsSequence([ - ("adb -s 0123456789abcdef shell " - "'cat \"/this/file/must.be.read.by.root\" 2> /dev/null'", - 'this is a test file\nread by root')]): + (self.call.device.NeedsSU(), True), + (self.call.adb.Shell("su -c sh -c 'cat /this/file/can.be.read.with.su'"), + 'this is a test file\nread with su')): self.assertEqual( - ['this is a test file', 'read by root'], - self.device.ReadFile('/this/file/must.be.read.by.root', - as_root=True)) - - def testReadFile_asRoot_withSu(self): - self.device.old_interface._privileged_command_runner = ( - self.device.old_interface.RunShellCommandWithSU) - self.device.old_interface._protected_file_access_method_initialized = True - with self.assertCallsSequence([ - ("adb -s 0123456789abcdef shell " - "'su -c cat \"/this/file/can.be.read.with.su\" 2> /dev/null'", - 'this is a test file\nread with su')]): - self.assertEqual( - ['this is a test file', 'read with su'], + 'this is a test file\nread with su\n', self.device.ReadFile('/this/file/can.be.read.with.su', as_root=True)) - def testReadFile_asRoot_rejected(self): - self.device.old_interface._privileged_command_runner = None - self.device.old_interface._protected_file_access_method_initialized = True - with self.assertRaises(device_errors.CommandFailedError): - self.device.ReadFile('/this/file/cannot.be.read.by.user', - as_root=True) - class DeviceUtilsWriteFileTest(DeviceUtilsNewImplTest): diff --git a/android/pylib/flag_changer.py b/android/pylib/flag_changer.py index c0bcadbe3..718bc3971 100644 --- a/android/pylib/flag_changer.py +++ b/android/pylib/flag_changer.py @@ -7,6 +7,8 @@ import logging import pylib.android_commands import pylib.device.device_utils +from pylib.device import device_errors + class FlagChanger(object): """Changes the flags Chrome runs with. @@ -32,9 +34,10 @@ class FlagChanger(object): self._cmdline_file = cmdline_file # Save the original flags. - self._orig_line = self._device.ReadFile(self._cmdline_file) - if self._orig_line: - self._orig_line = self._orig_line[0].strip() + try: + self._orig_line = self._device.ReadFile(self._cmdline_file).strip() + except device_errors.CommandFailedError: + self._orig_line = '' # Parse out the flags into a list to facilitate adding and removing flags. self._current_flags = self._TokenizeFlags(self._orig_line) @@ -104,8 +107,8 @@ class FlagChanger(object): self._device.WriteFile( self._cmdline_file, cmd_line, as_root=use_root) file_contents = self._device.ReadFile( - self._cmdline_file, as_root=use_root) - assert len(file_contents) == 1 and file_contents[0] == cmd_line, ( + self._cmdline_file, as_root=use_root).rstrip() + assert file_contents == cmd_line, ( 'Failed to set the command line file at %s' % self._cmdline_file) else: self._device.RunShellCommand('rm ' + self._cmdline_file, diff --git a/android/pylib/instrumentation/test_runner.py b/android/pylib/instrumentation/test_runner.py index ec0baeea1..424dcb3b1 100644 --- a/android/pylib/instrumentation/test_runner.py +++ b/android/pylib/instrumentation/test_runner.py @@ -237,10 +237,8 @@ class TestRunner(base_test_runner.BaseTestRunner): '/data/data/com.google.android.apps.chrome/files/PerfTestData.txt', as_root=True) - if json_string: - json_string = '\n'.join(json_string) - else: - raise Exception('Perf file does not exist or is empty') + if not json_string: + raise Exception('Perf file is empty') if self.options.save_perf_json: json_local_file = '/tmp/chromium-android-perf-json-' + raw_test_name diff --git a/android/pylib/perf/thermal_throttle.py b/android/pylib/perf/thermal_throttle.py index 24e1ff4c0..383b6d55f 100644 --- a/android/pylib/perf/thermal_throttle.py +++ b/android/pylib/perf/thermal_throttle.py @@ -34,7 +34,7 @@ class OmapThrottlingDetector(object): def GetCurrentTemperature(self): tempdata = self._device.ReadFile(OmapThrottlingDetector.OMAP_TEMP_FILE) - return float(tempdata[0]) / 1000.0 + return float(tempdata) / 1000.0 class ExynosThrottlingDetector(object): diff --git a/android/tombstones.py b/android/tombstones.py index a78d89fa5..7449a73b9 100755 --- a/android/tombstones.py +++ b/android/tombstones.py @@ -63,7 +63,8 @@ def _GetTombstoneData(device, tombstone_file): Returns: A list of lines """ - return device.ReadFile('/data/tombstones/' + tombstone_file, as_root=True) + return device.ReadFile( + '/data/tombstones/' + tombstone_file, as_root=True).splitlines() def _EraseTombstone(device, tombstone_file):