Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
251743003:
telemetry: android: use both methods to access protected files (Closed)
|
Created:
6 years, 8 months ago by pasko Modified:
6 years, 7 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, Fabrice (no longer in Chrome), Philippe Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptiontelemetry: android: use both methods to access protected files
Currently when telemetry asks to read/write a protected file, it attempts doing
so with the 'su' command, but not all devices have the 'su' command installed.
In this case telemetry ignores all errors and continues silently.
This change:
1. chooses the best method to access protected files: 'adb root' or 'su'
2. caches the choice
3. throws warnings each time a protected file cannot be accessed
BUG=366640
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267137
Patch Set 1 #
Total comments: 24
Patch Set 2 : comments #
Total comments: 3
Patch Set 3 : self._privileged_command_runner #
Total comments: 2
Patch Set 4 : lambda was not necessary #
Total comments: 2
Patch Set 5 : even more compact #
Total comments: 3
Messages
Total messages: 30 (0 generated)
I am no good python stylist, so please be picky :)
Philippe, can you take a look as well? As we discussed offline, I could probably replace AuxContentsLookReal with checking the exit code from a shell command, but now as I am looking at it, checking the contents of the file looks more robust to me, even though a hack. WDYT?
lgtm, I like this, thanks egor! some suggestions below that you may want to do as a follow up. I'm adding john as he's now the owner of android_commands and is in the middle of a great cleanup, please wait for his thoughts too.. https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1109: ACCESS_METHOD_WITH_SU = "with su" nit: use single quotes https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1129: if self._protected_file_access_method is not None: nit: s/is not None// https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1132: def AuxContentsLookReal(contents): nit: how about "_IsValidContents" ? https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1167: return [] now, if we're going to fix this once and for all :) frankf / craigdh had this idea of never failing gracefully at this level and instead throw hard exceptions, so that caller code can explicitly handle it. I'm not sure how much would it break right now, but I think we should at least give it a try(job), wdyt? https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1190: logging.warning('Could not set contents of protected file: %s' % filename) ditto.
https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1107: ACCESS_METHOD_NO_ACCESS = "no access" nit: Start with underscore since these probably don't need to be public. Also these could just be numeric constants since it doesn't look like the strings are used for anything. https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1142: contents = self.RunShellCommand('cat /proc/1/auxv') Neat trick :) https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1152: return self._protected_file_access_method Set access method to ACCESS_METHOD_NO_ACCESS here? https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1184: access_method = self._ProtectedFileAccessMethod() This could be a little cleaner if access_method was just a reference to self.RunShellCommand or self.RunShellCommandWithSU depending on the detection result. Then this could just check that it has valid access method and fail otherwise.
lgtm I'll add this functionality to the refactor. https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1107: ACCESS_METHOD_NO_ACCESS = "no access" On 2014/04/28 12:55:49, Sami wrote: > nit: Start with underscore since these probably don't need to be public. Also > these could just be numeric constants since it doesn't look like the strings are > used for anything. +1 for using numeric constants. https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1132: def AuxContentsLookReal(contents): Why check the output rather than the return code? https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1145: self._protected_file_access_method = AndroidCommands.ACCESS_METHOD_AS_ROOT FWIW, I've been toying with doing the as_root check in the refactored implementation by checking the 'service.adb.root' property on the device. That method doesn't have a great way to check for su access, though. https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1167: return [] On 2014/04/28 12:21:48, bulach wrote: > now, if we're going to fix this once and for all :) > frankf / craigdh had this idea of never failing gracefully at this level and > instead throw hard exceptions, so that caller code can explicitly handle it. > I'm not sure how much would it break right now, but I think we should at least > give it a try(job), wdyt? I'm ok with this idea in general. I think implementing it will break a lot, though, so I'm not sure that this CL is the place for it.
PTAL https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1107: ACCESS_METHOD_NO_ACCESS = "no access" On 2014/04/28 12:55:49, Sami wrote: > nit: Start with underscore since these probably don't need to be public. Also > these could just be numeric constants since it doesn't look like the strings are > used for anything. Renaming: Done. Most votes are for numeric constants, changed to numeric. https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1107: ACCESS_METHOD_NO_ACCESS = "no access" On 2014/04/28 14:22:52, jbudorick wrote: > On 2014/04/28 12:55:49, Sami wrote: > > nit: Start with underscore since these probably don't need to be public. Also > > these could just be numeric constants since it doesn't look like the strings > are > > used for anything. > > +1 for using numeric constants. Done. https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1109: ACCESS_METHOD_WITH_SU = "with su" On 2014/04/28 12:21:48, bulach wrote: > nit: use single quotes thanks, changed to numeric constants https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1129: if self._protected_file_access_method is not None: On 2014/04/28 12:21:48, bulach wrote: > nit: s/is not None// I changed this to numeric constants, so to distinguish between 0 and None we would need this 'is not None'. https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1132: def AuxContentsLookReal(contents): On 2014/04/28 12:21:48, bulach wrote: > nit: how about "_IsValidContents" ? I changed this to IsValidAuxContents(). Since the definition is function-local, we don't need the leading underscore, right? https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1132: def AuxContentsLookReal(contents): On 2014/04/28 14:22:52, jbudorick wrote: > Why check the output rather than the return code? This is mainly because RunShellCommand() and RunShellCommandWithSU() both return contents and swallow the return code. I did not want to risk potential future inconsistency between how I run "su" and how RunShellCommandWithSU() does it. However, the risk is relatively small and I could just do cmd_helper.RunCmd('su -c cat /proc/1/auxv') and check for the return code. I'm OK to use this variant if it's your preference. https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1142: contents = self.RunShellCommand('cat /proc/1/auxv') On 2014/04/28 12:55:49, Sami wrote: > Neat trick :) thnx) https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1145: self._protected_file_access_method = AndroidCommands.ACCESS_METHOD_AS_ROOT On 2014/04/28 14:22:52, jbudorick wrote: > FWIW, I've been toying with doing the as_root check in the refactored > implementation by checking the 'service.adb.root' property on the device. That > method doesn't have a great way to check for su access, though. I did not know about this property :) https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1152: return self._protected_file_access_method On 2014/04/28 12:55:49, Sami wrote: > Set access method to ACCESS_METHOD_NO_ACCESS here? I've set it above. If I wanted to move it here from the above, I'd have to add two more return statements. Is that what you want? https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1167: return [] On 2014/04/28 12:21:48, bulach wrote: > now, if we're going to fix this once and for all :) > frankf / craigdh had this idea of never failing gracefully at this level and > instead throw hard exceptions, so that caller code can explicitly handle it. > I'm not sure how much would it break right now, but I think we should at least > give it a try(job), wdyt? I'm +1 to jbudorick@ here: it will likely break in many places with a hard exception. Let's start with a mere warning and then address this in next CL(s). https://codereview.chromium.org/251743003/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1184: access_method = self._ProtectedFileAccessMethod() On 2014/04/28 12:55:49, Sami wrote: > This could be a little cleaner if access_method was just a reference to > self.RunShellCommand or self.RunShellCommandWithSU depending on the detection > result. Then this could just check that it has valid access method and fail > otherwise. Do you suggest to check for None and call otherwise? If i'd be checking for a method among RunShellCommand and RunShellCommandWithSU, then it's probably a little cleaner, but similar level of verbosity. I don't usually compare function pointers in python, but when I do, I ask if an average reader in our project would know what this magic means :)
On 2014/04/29 09:42:32, pasko wrote:
> Do you suggest to check for None and call otherwise? If i'd be checking for a
> method among RunShellCommand and RunShellCommandWithSU, then it's probably a
> little cleaner, but similar level of verbosity.
>
> I don't usually compare function pointers in python, but when I do, I ask if
an
> average reader in our project would know what this magic means :)
I don't think there's any need to compare pointers. I basically meant something
like this, in _GetProtectedFileAccessMethod:
if <protected data available directly>:
self._privileged_shell_command_runner = self.RunShellCommand
elif <protected data available through su>:
self._privileged_shell_command_runner = self.RunShellCommandSU
else:
self._privileged_shell_command_runner = None
Then in SetProtectedFileContents you could just do:
if self._privileged_shell_command_runner:
self._privileged_shell_command_runner(cmd)
else:
logging.warning(...)
Functions are first class objects in python so I don't think this is
particularly black magic.
still lgtm with some more suggestions, thanks! https://codereview.chromium.org/251743003/diff/20001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/20001/build/android/pylib/andr... build/android/pylib/android_commands.py:1107: _ACCESS_METHOD_NO_ACCESS = 0 nit: you may as well start with 1 (and if you like starting 0, have a _ACCESS_METHOD_UNDEFINED value), so that can remove is not None from 1129.. totally optional, but in general python treads 0, None, False, [] and '' as the same boolean value. https://codereview.chromium.org/251743003/diff/20001/build/android/pylib/andr... build/android/pylib/android_commands.py:1132: def IsValidAuxContents(contents): I'm not sure it'd be more readable, but how about: def ValidateContents(self, contents, method): if len(contents) > 0 ...: self._protected_file_access_method = method return True return False then later on it can have less indentation, like: self._protected_file_access_method = ...NO_ACCESS contents = self.RunShellCommand(...) if not ValidateContents(contents, ...AS_ROOT): contents = self.RunShellCommandWithSU(...) ValidateContent(contents, ...WITH_SU) return self._protected_file_access_method ...no idea if it's any clearer..
On 2014/04/29 10:16:34, Sami wrote: > On 2014/04/29 09:42:32, pasko wrote: > > Do you suggest to check for None and call otherwise? If i'd be checking for a > > method among RunShellCommand and RunShellCommandWithSU, then it's probably a > > little cleaner, but similar level of verbosity. > > > > I don't usually compare function pointers in python, but when I do, I ask if > an > > average reader in our project would know what this magic means :) > > I don't think there's any need to compare pointers. I basically meant something > like this, in _GetProtectedFileAccessMethod: > > if <protected data available directly>: > self._privileged_shell_command_runner = self.RunShellCommand > elif <protected data available through su>: > self._privileged_shell_command_runner = self.RunShellCommandSU > else: > self._privileged_shell_command_runner = None > > Then in SetProtectedFileContents you could just do: > > if self._privileged_shell_command_runner: > self._privileged_shell_command_runner(cmd) > else: > logging.warning(...) > > Functions are first class objects in python so I don't think this is > particularly black magic. I like that! Thanks! Hm, this specific proposal merges two concepts: availability of the access method on the device and having the command runner initialized. This indeed can be merged without much trouble: re-detect the runner every time the access method is not known, but that's not very intuitive :/ So I made something else with lambda. It's probably not much more readable, but certainly I do enjoy it more than the 3 constants. So be it.
https://codereview.chromium.org/251743003/diff/20001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/20001/build/android/pylib/andr... build/android/pylib/android_commands.py:1132: def IsValidAuxContents(contents): On 2014/04/29 10:21:39, bulach wrote: > I'm not sure it'd be more readable, but how about: > > def ValidateContents(self, contents, method): > if len(contents) > 0 ...: > self._protected_file_access_method = method > return True > return False > > then later on it can have less indentation, like: > self._protected_file_access_method = ...NO_ACCESS > contents = self.RunShellCommand(...) > if not ValidateContents(contents, ...AS_ROOT): > contents = self.RunShellCommandWithSU(...) > ValidateContent(contents, ...WITH_SU) > > return self._protected_file_access_method > > ...no idea if it's any clearer.. Maybe. This method is elegant, but introduces a side effect, and I could not find a good name for it. So not taking this valuable recommendation :)
Thanks, this looks much clearer. One question below. Also, there's a @_Memoized decorator in constants.py which you could use to remove the has_initialized state: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... If you're feeling charitable please move that to ./utils/ and reuse here. Happy with the existing approach too :) https://codereview.chromium.org/251743003/diff/40001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/40001/build/android/pylib/andr... build/android/pylib/android_commands.py:1143: self._privileged_command_runner = lambda cmd: self.RunShellCommand(cmd) Is the lambda necessary? I believe self._privileged_command_runner = self.RunShellCommand is equivalent. Python's bound functions are handy for this sort of stuff.
On 2014/04/29 13:16:19, Sami wrote: > Thanks, this looks much clearer. One question below. > > Also, there's a @_Memoized decorator in constants.py which you could use to > remove the has_initialized state: > > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... > > If you're feeling charitable please move that to ./utils/ and reuse here. Happy > with the existing approach too :) I am in the be-more-explicit mood today, sorry :) Would prefer not to use our custom decorator for saving 3 easy lines of code. That decorator also looks dangerous to me... what if the '_result' member is used for something else?
https://codereview.chromium.org/251743003/diff/40001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/40001/build/android/pylib/andr... build/android/pylib/android_commands.py:1143: self._privileged_command_runner = lambda cmd: self.RunShellCommand(cmd) On 2014/04/29 13:16:19, Sami wrote: > Is the lambda necessary? I believe self._privileged_command_runner = > self.RunShellCommand is equivalent. Python's bound functions are handy for this > sort of stuff. Wow, thanks! I had no idea. Done.
The CQ bit was checked by pasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@chromium.org/251743003/50002
still lgtm, just clarifying my suggestion and taking into account the lambda goodness... as before, feel free to ignore... https://codereview.chromium.org/251743003/diff/50002/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/50002/build/android/pylib/andr... build/android/pylib/android_commands.py:1149: return self._privileged_command_runner well, there's some small duplication here, we could minimize it by naming the function a "Setter" or something... the cat command, the if and the actual self.RunShell... would appear only once, something like: def SetPriviledgedCommandRunner(self, fn): contents = fn('cat /proc/1/auxv') if len(contents) and ...: self._priviledged_command_runner = fn return True return False if not self.SetPriviledgedCommandRunner(self, self.RunShellCommand) and not self.SetPriviledgedCommandRunner(self, self.RunShellCommandWithSU): logging.warning('Could not ...') self._priviledged_command_runner = lambda(x, y: return []) return self._priviledged_command_runner
https://codereview.chromium.org/251743003/diff/50002/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/50002/build/android/pylib/andr... build/android/pylib/android_commands.py:1149: return self._privileged_command_runner On 2014/04/29 13:42:02, bulach wrote: > well, there's some small duplication here, we could minimize it by naming the > function a "Setter" or something... the cat command, the if and the actual > self.RunShell... would appear only once, something like: > > def SetPriviledgedCommandRunner(self, fn): > contents = fn('cat /proc/1/auxv') > if len(contents) and ...: > self._priviledged_command_runner = fn > return True > return False > > if not self.SetPriviledgedCommandRunner(self, self.RunShellCommand) and > not self.SetPriviledgedCommandRunner(self, self.RunShellCommandWithSU): > logging.warning('Could not ...') > self._priviledged_command_runner = lambda(x, y: return []) > return self._priviledged_command_runner oh, that's more elegant than I thought it would be. I would like the warning to say what file we failed on, but that's orthogonal. There is yet another possibility: def PrivilegedCommandWorks(self, command): contents = command('cat /proc/1/auxv') return len(contents) > 0 and (contents[0][2] == '\0') self._privileged_command_runner = None for cmd in [self.RunShellCommand, self.RunShellCommandWithSU]: if PrivilegedCommandWorks(cmd): self._privileged_command_runner = cmd break return self._privileged_command_runner
The CQ bit was unchecked by pasko@chromium.org
bulach: repetitions eliminated, function removed, yey, ptal
still lgtm :) https://codereview.chromium.org/251743003/diff/70001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/70001/build/android/pylib/andr... build/android/pylib/android_commands.py:1130: self._protected_file_access_method_initialized = True nit: I think you can get away with the boolean. after the loop below, and initialize with a "lambda x, y: []" and log there.. this function is not taking into account which file is trying to access anyways, so logging the file name in the outer function is a bit moot.. https://codereview.chromium.org/251743003/diff/70001/build/android/pylib/andr... build/android/pylib/android_commands.py:1154: logging.warning('Could not access protected file: %s' % filename) if you agree with the above comment about the log, then this can be simply: command = ... command_runner = ... return command_runner(command)
https://codereview.chromium.org/251743003/diff/70001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/251743003/diff/70001/build/android/pylib/andr... build/android/pylib/android_commands.py:1130: self._protected_file_access_method_initialized = True On 2014/04/29 14:26:54, bulach wrote: > nit: I think you can get away with the boolean. > after the loop below, and initialize with a "lambda x, y: []" and log there.. > this function is not taking into account which file is trying to access anyways, > so logging the file name in the outer function is a bit moot.. I did not quite understand this statement :) The logging happens in two situations: 1. GetProtectedFileContents() 2. SetProtectedFileContents() I want the warning message to mention the filename *and* the type of operation performed *and* each time it happens (not only the first time). This place does not have enough context for the purpose.
The CQ bit was checked by pasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@chromium.org/251743003/70001
On 2014/04/29 15:39:16, pasko wrote: > https://codereview.chromium.org/251743003/diff/70001/build/android/pylib/andr... > File build/android/pylib/android_commands.py (right): > > https://codereview.chromium.org/251743003/diff/70001/build/android/pylib/andr... > build/android/pylib/android_commands.py:1130: > self._protected_file_access_method_initialized = True > On 2014/04/29 14:26:54, bulach wrote: > > nit: I think you can get away with the boolean. > > after the loop below, and initialize with a "lambda x, y: []" and log there.. > > this function is not taking into account which file is trying to access > anyways, > > so logging the file name in the outer function is a bit moot.. > > I did not quite understand this statement :) > The logging happens in two situations: > 1. GetProtectedFileContents() > 2. SetProtectedFileContents() > > I want the warning message to mention the filename *and* the type of operation > performed *and* each time it happens (not only the first time). This place does > not have enough context for the purpose. still lgtm :) but to clarify: what you described seems a bit spammy :) given that the result won't ever change [1], it seems unnecessary to log each filename *and* type of operation.. [1] in an ideal world, we'd always have root, or su, or nothing, as an invariant throughout the test. of course, the root-ness may change, but I'd consider that a problem to solve during provision..
On 2014/04/29 16:04:41, bulach wrote: > On 2014/04/29 15:39:16, pasko wrote: > > > https://codereview.chromium.org/251743003/diff/70001/build/android/pylib/andr... > > File build/android/pylib/android_commands.py (right): > > > > > https://codereview.chromium.org/251743003/diff/70001/build/android/pylib/andr... > > build/android/pylib/android_commands.py:1130: > > self._protected_file_access_method_initialized = True > > On 2014/04/29 14:26:54, bulach wrote: > > > nit: I think you can get away with the boolean. > > > after the loop below, and initialize with a "lambda x, y: []" and log > there.. > > > this function is not taking into account which file is trying to access > > anyways, > > > so logging the file name in the outer function is a bit moot.. > > > > I did not quite understand this statement :) > > The logging happens in two situations: > > 1. GetProtectedFileContents() > > 2. SetProtectedFileContents() > > > > I want the warning message to mention the filename *and* the type of operation > > performed *and* each time it happens (not only the first time). This place > does > > not have enough context for the purpose. > > still lgtm :) but to clarify: > what you described seems a bit spammy :) > given that the result won't ever change [1], it seems unnecessary to log each > filename *and* type of operation.. > > [1] in an ideal world, we'd always have root, or su, or nothing, as an invariant > throughout the test. > of course, the root-ness may change, but I'd consider that a problem to solve > during provision.. yeah, the warning is to estimate how many broken places we have, and be able to fix them all by looking at one log. Yes, it's spammy, but the amount of spam will go down with time .. according to the plan.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
The CQ bit was checked by pasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@chromium.org/251743003/70001
Message was sent while issue was closed.
Change committed as 267137 |
