Factor out a system_properties interface for interacting with getprop/setprop.

Factor out a system_properties interface for interacting with getprop/setprop.

Also, teach it to cache read-only properties so that we don't hit the device so
often. This caching is a prereq for speed index support in Telemetry which would
like to query these often.

BUG=323813
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238601

[frankf, 2013-12-03 00:20:27]

Nice refactoring

https://codereview.chromium.org/99713002/diff/1/build/android/pylib/android_c...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/99713002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:599: 'setprop':
'system_properties[<PROPERTY>]',
Could you add 'su' to this list? People should be using RunShellCommandWithSU()

https://codereview.chromium.org/99713002/diff/1/build/android/pylib/system_pr...
File build/android/pylib/system_properties.py (right):

https://codereview.chromium.org/99713002/diff/1/build/android/pylib/system_pr...
build/android/pylib/system_properties.py:10: System properties are key/value
pairs as expose by adb shell getprop/setprop.
exposed

https://codereview.chromium.org/99713002/diff/1/build/android/pylib/system_pr...
build/android/pylib/system_properties.py:29: self._adb.SendShellCommand('setprop
%s "%s"' % (key, value), retry_count=3)
We're trying be more defensive wrt android commands. So be mindful of how things
could silently fail.

One case I can think of is when device is not root. setprop will succeed. So
perhaps, you want to verify the value is actually set.

[tonyg, 2013-12-03 01:02:01]

https://codereview.chromium.org/99713002/diff/1/build/android/pylib/android_c...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/99713002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:599: 'setprop':
'system_properties[<PROPERTY>]',
On 2013/12/03 00:20:27, frankf wrote:
> Could you add 'su' to this list? People should be using
RunShellCommandWithSU()

Done.

https://codereview.chromium.org/99713002/diff/1/build/android/pylib/system_pr...
File build/android/pylib/system_properties.py (right):

https://codereview.chromium.org/99713002/diff/1/build/android/pylib/system_pr...
build/android/pylib/system_properties.py:10: System properties are key/value
pairs as expose by adb shell getprop/setprop.
On 2013/12/03 00:20:27, frankf wrote:
> exposed

Done.

https://codereview.chromium.org/99713002/diff/1/build/android/pylib/system_pr...
build/android/pylib/system_properties.py:29: self._adb.SendShellCommand('setprop
%s "%s"' % (key, value), retry_count=3)
On 2013/12/03 00:20:27, frankf wrote:
> We're trying be more defensive wrt android commands. So be mindful of how
things
> could silently fail.
> 
> One case I can think of is when device is not root. setprop will succeed. So
> perhaps, you want to verify the value is actually set.

Done.

[frankf, 2013-12-03 01:27:28]

https://codereview.chromium.org/99713002/diff/20001/build/android/pylib/syste...
File build/android/pylib/system_properties.py (right):

https://codereview.chromium.org/99713002/diff/20001/build/android/pylib/syste...
build/android/pylib/system_properties.py:30: 'setprop %s "%s"; echo $?' % (key,
value), retry_count=3).strip()
I tried on KRT16M and setprop returns 0 even if doesn't set the value because
the device is not rooted. In fact it seems to always return 0. E.g. 

$ adb shell setprop blah; echo $?

usage: setprop <key> <value>
0

[tonyg, 2013-12-03 02:38:51]

On 2013/12/03 01:27:28, frankf wrote:
>
https://codereview.chromium.org/99713002/diff/20001/build/android/pylib/syste...
> File build/android/pylib/system_properties.py (right):
> 
>
https://codereview.chromium.org/99713002/diff/20001/build/android/pylib/syste...
> build/android/pylib/system_properties.py:30: 'setprop %s "%s"; echo $?' %
(key,
> value), retry_count=3).strip()
> I tried on KRT16M and setprop returns 0 even if doesn't set the value because
> the device is not rooted. In fact it seems to always return 0. E.g. 
> 
> $ adb shell setprop blah; echo $?
> 
> usage: setprop <key> <value>
> 0

You are right.

I tried to get this to work, but haven't figured anything out yet. The wrinkle
is that one of the things Telemetry does is set sys.usb.config. This momentarily
breaks the adb connection and means that getprop cannot be read back
immediately. Telemetry has a WaitFor() to wait for it to come back after this,
but that doesn't seem to belong in this method.

So, if you are okay with it, I'd like to punt. The behavior of this patch
matches the previous behavior and at least now how know we have a single place
to make more robust. I've added a TODO.

[bulach, 2013-12-03 14:50:48]

lgtm, just some suggestions (and please make sure frank is happy too!)

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/andro...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/andro...
build/android/pylib/android_commands.py:630: self._CheckCommandIsValid(command)
why not just add this check to RunShellCommand and avoid having this internal
method?

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/andro...
build/android/pylib/android_commands.py:1045: self._CheckCommandIsValid(command)
see above..

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/syste...
File build/android/pylib/system_properties.py (right):

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/syste...
build/android/pylib/system_properties.py:29: # TODO(tonyg): This can fail with
no root. Verify that it succeeds.
nit: perhaps
assert self._adb.IsRootEnabled()

[tonyg, 2013-12-03 15:29:36]

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/andro...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/andro...
build/android/pylib/android_commands.py:630: self._CheckCommandIsValid(command)
On 2013/12/03 14:50:49, bulach wrote:
> why not just add this check to RunShellCommand and avoid having this internal
> method?

Because RunShellCommandWithSU calls RunShellCommand and CheckCommandIsValid
prohibits su.

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/andro...
build/android/pylib/android_commands.py:1045: self._CheckCommandIsValid(command)
On 2013/12/03 14:50:49, bulach wrote:
> see above..

ditto

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/syste...
File build/android/pylib/system_properties.py (right):

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/syste...
build/android/pylib/system_properties.py:29: # TODO(tonyg): This can fail with
no root. Verify that it succeeds.
On 2013/12/03 14:50:49, bulach wrote:
> nit: perhaps
> assert self._adb.IsRootEnabled()

If you guys would be amenable, I'd like to just leave the current behavior alone
in this patch. Because if the new assertion hits, this whole patch will probably
get reverted. We can experiment with adding the assertion subsequently.

[bulach, 2013-12-03 15:39:30]

still lgtm, but one suggestion:

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/andro...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/andro...
build/android/pylib/android_commands.py:630: self._CheckCommandIsValid(command)
On 2013/12/03 15:29:36, tonyg wrote:
> On 2013/12/03 14:50:49, bulach wrote:
> > why not just add this check to RunShellCommand and avoid having this
internal
> > method?
> 
> Because RunShellCommandWithSU calls RunShellCommand and CheckCommandIsValid
> prohibits su.

how about assert that if RunShellCommand's command has "su" then it's caller is
indeed RunShellCommandWithSU?

import inspect

caller = inspect.stack()[1][3]

self._CheckCommandIsValid(command, caller)


my point is that having both RunShellCommand and _RunShellCommand would be prone
to abuse and confusion, so I'd rather keep it simple and even more explicit
about the tests... wdyt?

[tonyg, 2013-12-03 17:44:31]

On 2013/12/03 15:39:30, bulach wrote:
> still lgtm, but one suggestion:
> 
>
https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/andro...
> File build/android/pylib/android_commands.py (right):
> 
>
https://codereview.chromium.org/99713002/diff/40001/build/android/pylib/andro...
> build/android/pylib/android_commands.py:630:
self._CheckCommandIsValid(command)
> On 2013/12/03 15:29:36, tonyg wrote:
> > On 2013/12/03 14:50:49, bulach wrote:
> > > why not just add this check to RunShellCommand and avoid having this
> internal
> > > method?
> > 
> > Because RunShellCommandWithSU calls RunShellCommand and CheckCommandIsValid
> > prohibits su.
> 
> how about assert that if RunShellCommand's command has "su" then it's caller
is
> indeed RunShellCommandWithSU?
> 
> import inspect
> 
> caller = inspect.stack()[1][3]
> 
> self._CheckCommandIsValid(command, caller)
> 
> 
> my point is that having both RunShellCommand and _RunShellCommand would be
prone
> to abuse and confusion, so I'd rather keep it simple and even more explicit
> about the tests... wdyt?

Done. PTAL

[bulach, 2013-12-03 17:50:17]

lgtm, thanks!

https://codereview.chromium.org/99713002/diff/60001/build/android/pylib/andro...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/99713002/diff/60001/build/android/pylib/andro...
build/android/pylib/android_commands.py:609: base_command = command.split()[0]
ultra nit: it'd be moar correct to shlex.split(command)[0]

[tonyg, 2013-12-03 17:53:42]

On 2013/12/03 17:50:17, bulach wrote:
> lgtm, thanks!
> 
>
https://codereview.chromium.org/99713002/diff/60001/build/android/pylib/andro...
> File build/android/pylib/android_commands.py (right):
> 
>
https://codereview.chromium.org/99713002/diff/60001/build/android/pylib/andro...
> build/android/pylib/android_commands.py:609: base_command = command.split()[0]
> ultra nit: it'd be moar correct to shlex.split(command)[0]

Fixed. I'll hold off for Frank's approval now.

[frankf, 2013-12-03 18:44:06]

lgtm with a suggestion for a follow-up

https://codereview.chromium.org/99713002/diff/70001/build/android/pylib/syste...
File build/android/pylib/system_properties.py (right):

https://codereview.chromium.org/99713002/diff/70001/build/android/pylib/syste...
build/android/pylib/system_properties.py:29: # TODO(tonyg): This can fail with
no root. Verify that it succeeds.
In a future patch you can add an optional parameter verify=True which you can
set to False for that corner case.

[commit-bot: I haz the power, 2013-12-03 18:58:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/99713002/70001

[commit-bot: I haz the power, 2013-12-03 19:19:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/99713002/70001

[commit-bot: I haz the power, 2013-12-03 23:51:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/99713002/70001

[commit-bot: I haz the power, 2013-12-04 06:23:47]

Change committed as 238601