Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
715853002:
Migrate device utils WriteFile to AdbWrapper (Closed)
|
Created:
6 years, 1 month ago by perezju Modified:
6 years, 1 month ago CC:
chromium-reviews, klundberg+watch_chromium.org, telemetry+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMigrate device utils WriteFile to AdbWrapper
Also merged the functionality of WriteFile and WriteTextFile into a
single method.
BUG=267773
Committed: https://crrev.com/6de48323f78311342108fda8b2f59ba3848eea81
Cr-Commit-Position: refs/heads/master@{#303902}
Patch Set 1 #
Total comments: 5
Patch Set 2 : fixed edge cases #
Total comments: 13
Patch Set 3 : added tests with large contents #Patch Set 4 : fixed another echo bug #
Total comments: 3
Patch Set 5 : fixed bug running shell commands with su #
Total comments: 3
Messages
Total messages: 19 (2 generated)
perezju@chromium.org changed reviewers: + jbudorick@chromium.org, skyostil@chromium.org
ptal I've also merged WriteTextFile into WriteFile, and removed the couple of references to it from external clients. https://codereview.chromium.org/715853002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/715853002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:228: self._adb.device().WriteFile(cmdline_file, content, as_root=True) With the new semantics of as_root=True (use 'su' only if needed and available), this will "do the right thing" also when running on a user build. No more need to figure out the "correct" value to pass.
https://codereview.chromium.org/715853002/diff/1/build/android/pylib/device/d... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/715853002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:902: self.RunShellCommand(['mv', device_temp.name, device_path], Is DeviceTempFile ok with its temporary file getting moved out from under it? https://codereview.chromium.org/715853002/diff/1/build/android/pylib/device/d... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/715853002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils_test.py:1170: self.call.adb.Push('/tmp/file/on.host', '/path/to/device/file')): Just to be clear: this not being a tuple means that we don't do anything, right? (No return value, no exception, etc.?)
https://codereview.chromium.org/715853002/diff/1/build/android/pylib/device/d... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/715853002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:902: self.RunShellCommand(['mv', device_temp.name, device_path], On 2014/11/11 23:16:03, jbudorick wrote: > Is DeviceTempFile ok with its temporary file getting moved out from under it? The way it's now implemented, it should be fine. But, perhaps, we should add an explicit check_return=False in here: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... (or, to make it consistent with the future-planned-maybe-fate of check_return, instead of using this option add the try/except block to catch and dismiss the AdbShellCommandFailedError?) https://codereview.chromium.org/715853002/diff/1/build/android/pylib/device/d... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/715853002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils_test.py:1170: self.call.adb.Push('/tmp/file/on.host', '/path/to/device/file')): On 2014/11/11 23:16:03, jbudorick wrote: > Just to be clear: this not being a tuple means that we don't do anything, right? > (No return value, no exception, etc.?) Yep, causes the call to return None.
Ok, so I ran a bunch of end-to-end roundtrip tests locally on my machine with real devices, writing something to a file and then reading from it and check that we've got the same result. I ran these on some 'user' devices, as well as 'userdebug' with/without root, forcing to push or to echo, including quotes and even unicode characters encoded as bytes in the content. This revealed a couple of edge cases which I've fixed now. https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:901: self.RunShellCommand(sh_cmd, as_root=as_root, check_return=True) Not sure why this never hit us before! Probably WriteTextFile was only used on rooted devices, and the problem wouldn't show up. https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:913: as_root=True, check_return=True) So, I had to resort to 'cp' rather than 'mv' in the end. Still this makes me wonder, for the most common use cases, should temp files be written to /data/local/tmp or to external storage (as done now)? https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils_test.py:1208: """sh -c 'echo -n '"'"'the contents'"'"'' > '/test/file/to write'"""), And imagine trying to get those quotes right without cmd_helper.SingleQuote!
lgtm https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:901: self.RunShellCommand(sh_cmd, as_root=as_root, check_return=True) On 2014/11/12 01:31:24, perezju wrote: > Not sure why this never hit us before! Probably WriteTextFile was only used on > rooted devices, and the problem wouldn't show up. Wow, nice catch. This is a nasty little corner case. I wonder if there are other cases where we'd try to use echo on the device and, if so, if we should provide a utility function -- probably in here, but possibly in AdbWrapper -- to handle it. https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:913: as_root=True, check_return=True) On 2014/11/12 01:31:24, perezju wrote: > So, I had to resort to 'cp' rather than 'mv' in the end. Still this makes me > wonder, for the most common use cases, should temp files be written to > /data/local/tmp or to external storage (as done now)? While I think we should try to move away from using /data/local/tmp (as it's only readable from APKs, not writable), it would probably be ok to use it for DeviceTempFile. It'd be interesting to see if there's a noticeable difference in speed. However, as usual, not in this CL. https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils_test.py:1208: """sh -c 'echo -n '"'"'the contents'"'"'' > '/test/file/to write'"""), On 2014/11/12 01:31:24, perezju wrote: > And imagine trying to get those quotes right without cmd_helper.SingleQuote! Ha, no kidding.
Great find with the 'su' bug! https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:880: force_push: A boolean indicating whether to force the operation to be Is force_push needed by anything other than tests? If not, we should either rename it to force_push_for_testing for write the tests so that they trigger the appropriate code path automatically. (tip: you can easily create long strings in Python with multiplication, e.g., 'foo' * 64). https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:894: # won't work, because echo is interpreted by the shell and not an actual nit: I presume it's not the echo that gets executed but rather the output redirection (">"). https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils_test.py:1208: """sh -c 'echo -n '"'"'the contents'"'"'' > '/test/file/to write'"""), On 2014/11/12 01:42:38, jbudorick wrote: > On 2014/11/12 01:31:24, perezju wrote: > > And imagine trying to get those quotes right without cmd_helper.SingleQuote! > > Ha, no kidding. Should we add a couple of cases where the contents include quotes and angle brackets or is that already tested elsewhere?
just replying to some of your comments https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:880: force_push: A boolean indicating whether to force the operation to be On 2014/11/12 14:14:14, Sami wrote: > Is force_push needed by anything other than tests? If not, we should either > rename it to force_push_for_testing for write the tests so that they trigger the > appropriate code path automatically. (tip: you can easily create long strings in > Python with multiplication, e.g., 'foo' * 64). Well, yes, certainly it's useful for testing, but I thought that the option also makes sense, e.g., if one is trying to push binary data into the device and one is worried about the data being mangled by the shell (I think it "should" work anyway in most cases, I tested it at least with unicode chars and they made it through when passed by echo, but can't be sure about more complicated cases). Thanks for the tip anyway, I'll add some test cases using long contents rather than forcing the push with an option. https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:894: # won't work, because echo is interpreted by the shell and not an actual On 2014/11/12 14:14:14, Sami wrote: > nit: I presume it's not the echo that gets executed but rather the output > redirection (">"). No. "su -c cmd > /some/file" works fine, because the whole thing will be passed to adb shell, and the redirection is interpreted correctly. The problem is that "su" will attempt to execute "cmd" without starting a (new) shell, since in most cases it's not needed, and because echo does not exist as a command, it fails with the error: su: exec failed for echo Error:No such file or directory https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:901: self.RunShellCommand(sh_cmd, as_root=as_root, check_return=True) On 2014/11/12 01:42:37, jbudorick wrote: > On 2014/11/12 01:31:24, perezju wrote: > > Not sure why this never hit us before! Probably WriteTextFile was only used on > > rooted devices, and the problem wouldn't show up. > > Wow, nice catch. This is a nasty little corner case. I wonder if there are other > cases where we'd try to use echo on the device and, if so, if we should provide > a utility function -- probably in here, but possibly in AdbWrapper -- to handle > it. Indeed, echo (and sometimes with as_root=True!) is used in a couple other places https://code.google.com/p/chromium/codesearch#search/&q=adb%20echo%20file:%5C... Most of the times it's used to write something to a file, or sometimes to append to it. We should (in another CL) replace those clients with calls to WriteFile, and maybe think about an "append" option or an AppendFile method. When the output is not sent to a file, most likely it's being used to read the value from an environment variable, so we may also want to write a dedicated method for that at some point. https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/715853002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils_test.py:1208: """sh -c 'echo -n '"'"'the contents'"'"'' > '/test/file/to write'"""), On 2014/11/12 14:14:14, Sami wrote: > On 2014/11/12 01:42:38, jbudorick wrote: > > On 2014/11/12 01:31:24, perezju wrote: > > > And imagine trying to get those quotes right without cmd_helper.SingleQuote! > > > > Ha, no kidding. > > Should we add a couple of cases where the contents include quotes and angle > brackets or is that already tested elsewhere? Yeah, these are now being tested in pylib.cmd_helper_test, although we could add a couple more tests.
Uh, oh... just found another "su" bug. https://codereview.chromium.org/715853002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/715853002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:901: self.RunShellCommand(sh_cmd, as_root=as_root, check_return=True) what Sami said got me thinking and he was right, in part, the output redirection in my previous patch was "outside" of the su, so it wasn't being performed with privileges at all. The redirection must sit within the cmd passed to sh, which will be then run privileged when requested. I tested this now writing to a *protected* folder on a device (silly me for not doing it before!), and indeed was broken but now works as expected. Not sure if maybe we should be doing this (run "su -c sh -c ...") for *all* commands in RunShellCommand when su is needed. Thoughts?
https://codereview.chromium.org/715853002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/715853002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:901: self.RunShellCommand(sh_cmd, as_root=as_root, check_return=True) On 2014/11/12 17:59:48, perezju wrote: > what Sami said got me thinking and he was right, in part, the output redirection > in my previous patch was "outside" of the su, so it wasn't being performed with > privileges at all. The redirection must sit within the cmd passed to sh, which > will be then run privileged when requested. > > I tested this now writing to a *protected* folder on a device (silly me for not > doing it before!), and indeed was broken but now works as expected. Not sure if Thanks for checking :) Is it possible to have a test that does that for real or is everything mocked out in these tests? Btw, another way to do this without a subshell: "echo -n %s | su -c dd of=filename" % cmd_helper.SingleQuote(contents) > maybe we should be doing this (run "su -c sh -c ...") for *all* commands in > RunShellCommand when su is needed. Thoughts? Yeah, the name suggests that it supports running shell commands and not just binaries on the system, so doing that would make sense.
ptal https://codereview.chromium.org/715853002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/715853002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:901: self.RunShellCommand(sh_cmd, as_root=as_root, check_return=True) On 2014/11/12 18:23:10, Sami wrote: > Thanks for checking :) Is it possible to have a test that does that for real or > is everything mocked out in these tests? Everything in device_utils_tests is mocked at the AdbWrapper level, I assume so that tests can be run on presubmit without needing devices attached. John, should we also run some "real" tests automatically somewhere? (Maybe they already are? Where?) https://codereview.chromium.org/715853002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/715853002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:440: cmd = 'su -c sh -c %s' % cmd_helper.SingleQuote(cmd) I also moved the root check to the end so that, e.g., having the options cwd='/path/to/protected/folder' and as_root=True together work as expected. https://codereview.chromium.org/715853002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:895: self.RunShellCommand(cmd, as_root=as_root, check_return=True) Now RunShellCommand will "do the right thing", so no need for special treatment of the command here. This will also "automagically" fix the same bug for other clients attempting to run echo (or other shell features) through RunShellCommand with as_root=True. Thanks Sami for helping to spot this!
On 2014/11/12 19:30:41, perezju wrote: > ptal > > https://codereview.chromium.org/715853002/diff/60001/build/android/pylib/devi... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/715853002/diff/60001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:901: self.RunShellCommand(sh_cmd, > as_root=as_root, check_return=True) > On 2014/11/12 18:23:10, Sami wrote: > > Thanks for checking :) Is it possible to have a test that does that for real > or > > is everything mocked out in these tests? > > Everything in device_utils_tests is mocked at the AdbWrapper level, I assume so > that tests can be run on presubmit without needing devices attached. > > John, should we also run some "real" tests automatically somewhere? (Maybe they > already are? Where?) Testing for "real" as in something like DeviceUtils integration tests that make actual calls to devices? Nothing like that currently exists, and I'm not sure that the maintenance costs of those tests is worth it. > > https://codereview.chromium.org/715853002/diff/80001/build/android/pylib/devi... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/715853002/diff/80001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:440: cmd = 'su -c sh -c %s' % > cmd_helper.SingleQuote(cmd) > I also moved the root check to the end so that, e.g., having the options > cwd='/path/to/protected/folder' and as_root=True together work as expected. > > https://codereview.chromium.org/715853002/diff/80001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:895: self.RunShellCommand(cmd, > as_root=as_root, check_return=True) > Now RunShellCommand will "do the right thing", so no need for special treatment > of the command here. This will also "automagically" fix the same bug for other > clients attempting to run echo (or other shell features) through RunShellCommand > with as_root=True. > > Thanks Sami for helping to spot this!
Thanks Juan, lgtm. https://codereview.chromium.org/715853002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/715853002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:439: # "su -c sh -c" allows using shell features in |cmd| Random thought: subprocess lets you use shell=True/False to control this behavior. It's mostly for security, which isn't the top priority here.
The CQ bit was checked by perezju@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/715853002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6de48323f78311342108fda8b2f59ba3848eea81 Cr-Commit-Position: refs/heads/master@{#303902}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/725553002/ by skyostil@chromium.org. The reason for reverting is: Looks like this broke most of the perf tests on Nexus 4. Example log: http://build.chromium.org/p/chromium.perf/builders/Android%20Nexus4%20Perf/bu....
Message was sent while issue was closed.
Also seems to affect Nexus 7v2: http://build.chromium.org/p/chromium.perf/builders/Android%20Nexus7v2%20Perf/... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
