Eugh, filing fail. In discussing bug 754873, it came out that some commands that we might like to execute on a device require root permission (besides logcat, screencap, monkey, and my new orangatun come to mind). While theoretically you can do this with devicemanagerSUT by passing su -c to the command, in practice the quoting rules are annoying and difficult to get right. It would be better if we just executed things as root inside the agent.

So jmaher made devicemanagerSUT pass commands as root, and I fixed up the quoting some in bug 772595. However, it looks like we still want this, as running "su" resets the environment, which will cause commands like "am" and "logcat" to segfault on ICS-based devices like the Galaxy Nexus and Pandaboards.

Adding a few more folks who might care about this.

This was actually remarkably easy to implement and seems to work well for the basic cases I've tested (getting logcat, running fennec). Joel: Is there a way we could test this new agent with reftest, mochitest, etc. to make sure nothing breaks before I land this? I'm pretty positive it's working well.

(In reply to William Lachance (:wlach) from comment #4) > Joel: Is there a way we could test this new agent with reftest, mochitest, > etc. to make sure nothing breaks before I land this? I'm pretty positive > it's working well. I can deploy this to a subset of tegras if you provide the SUTAgent apk (with the same version would be easiest to test small number of tegras with, hardest to deploy/rollback -- with newer version most annoying to test with [because we only have so much space for staging] but easiest to rollback/deploy) I'll leave it to joel if he wants us to stage this seperately from a whole SUTagent upgrade.

Comment on attachment 640835 [details] [diff] [review] Patch to make exec use su by default Review of attachment 640835 [details] [diff] [review]: ----------------------------------------------------------------- hmm, would we need to remove the devicemanager part of this?

regarding sutagent rollout, I would prefer to do a local staging run. My biggest gripe about a staging run is we (as in myself or wlach) do not have a way to determine what is running the new agent and how to track the runs that it does. Also this would need to be in production instead of the 'staging' pool as history proves that staging pool is less reliable than production.

(In reply to Joel Maher (:jmaher) from comment #6) > Comment on attachment 640835 [details] [diff] [review] > Patch to make exec use su by default > > Review of attachment 640835 [details] [diff] [review]: > ----------------------------------------------------------------- > > hmm, would we need to remove the devicemanager part of this? That would be the ultimate goal (since it will fix the use of shell() on ICS-based devices), but in my limited testing I haven't seen a need to land both changse simultaneously. We should be able to update the agent, then land a patch to dmSUT later.

Ok, sounds like this should be ok. We'll test this change along with whatever else in staging before deployment, of course. Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d8c1913e669

So in further testing, it looks like the way we quote in devicemanager (") breaks when passed down to "su" (it just uses the first part of the command). The ideal solution would be to just remove the call to su altogether, but we can't do that until we're using the new agent in production. Attaching another patch to use the old-style quoting, which seems to work great for both launching fennec and doing a logcat (the two main things we are currently using exec for)

Followup patch to use a differnet quoting strategy.

(In reply to William Lachance (:wlach) from comment #12) > Created attachment 643623 [details] [diff] [review] > Use a different quoting strategy when calling exec with su > > Followup patch to use a differnet quoting strategy. Trying: https://tbpl.mozilla.org/?tree=Try&rev=c05cad2a9246

Ok, actually did some testing myself and found that this patch breaks logcat when using the old agent. :( I think the right thing here is to suffer through the old behaviour for now, wait for a new agent with my patch applied to be deployed, then remove the "su -c" part entirely. For now, the one thing that we need su for (logcat) seems to work ok with both versions of the agent.

Followup patch. Shouldn't be applied until we're using a new devicemanagerSUT

Comment on attachment 643649 [details] [diff] [review] Patch to remove su call from devicemanagerSUT Review of attachment 643649 [details] [diff] [review]: ----------------------------------------------------------------- nice!

Blocking bug 769428 since it tracks stability improvements for the panda images. Sorry for the noise.

Reverted patch to sutagent: https://hg.mozilla.org/integration/mozilla-inbound/rev/a672c266059f It was causing problems with pandas and tegras because of the way their su command works. For the pandas, we need to patch "su" to not prompt by default (example of this here: https://github.com/wlach/orangutan/blob/master/su.c). I am not sure what the problem is on the pandas, will have to debug further.

Moving to Testing/SUTAgent.

Ok, so it's looking like my earlier strategy caused breakage on the tegras or pandas or something (feedback from jmaher). I suspect the issue is with quoting, though I'm not sure. Unfortunately we just don't have time or resources to properly debug that. Let's take a different approach: add variants of the "exec" command that execute all commands as root, and make it possible to specify their use by passing a special argument to shell(). This should allow us to get access to these special privileges without trouble where we need it, and preserve backwards compatibility where required.

Comment on attachment 655595 [details] [diff] [review] Patch to add optional "execsu" command Review of attachment 655595 [details] [diff] [review]: ----------------------------------------------------------------- this approach is fine. I haven't tested this, but you could test this on try and later today or tomorrow I could run it on my circuit board farm. ::: build/mobile/devicemanagerSUT.py @@ +271,5 @@ > + cmd = "exec" > + if cwd: > + cmd += "cwd" > + if root: > + cmd += "su" please document that we are executing: 'exec', 'execsu', 'execcwd' or 'execcwdsu'

Try run for 128a0a829aaa is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=128a0a829aaa Results (out of 2 total builds): exception: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.com-128a0a829aaa

This patch adds the comment requested in the review, and also backwards compatibility with the old agent (so we don't have to gate committing this code on an updated agent)

Doing a try run: https://tbpl.mozilla.org/?tree=Try&rev=4b2f7621cf96

Comment on attachment 655607 [details] [diff] [review] 4th and hopefully final take on the patch Review of attachment 655607 [details] [diff] [review]: ----------------------------------------------------------------- a few logic issues in here and a few nits. ::: build/mobile/devicemanagerSUT.py @@ +55,5 @@ > if self.getDeviceRoot() == None: > + raise BaseException("Failed to connect to SUT Agent and retrieve the device root.") > + try: > + verstring = self.runCmds([{ 'cmd': 'ver' }]) > + self.agentVersion = re.sub('SUTAgentAndroid Version ', '', verstring) does this have any \n at the end or spaces we need to look out for? @@ +273,5 @@ > cmdline = self._escapedCommandLine(cmd) > if env: > cmdline = '%s %s' % (self.formatEnvString(env), cmdline) > > + haveExecSu = (StrictVersion(self.agentVersion) >= StrictVersion('1.12')) 1.13, 1.12 is already deployed @@ +291,3 @@ > try: > if cwd: > + self.sendCmds([{ 'cmd': '%s %s %s' % (cmd, cwd, cmdline) }], outputfile, timeout) what about execcwdsu? That should be limited to version 1.13 or higher. @@ +294,3 @@ > else: > + if not root or haveExecSu: > + self.sendCmds([{ 'cmd': '%s %s"' % (cmd, cmdline) }], outputfile, timeout) this will always execute if root=True, that won't work for backwards compatibility. ::: build/mobile/sutagent/android/DoCommand.java @@ +3820,4 @@ > "run [cmdline] - start program no wait\n" + > "exec [env pairs] [cmdline] - start program no wait optionally pass env\n" + > " key=value pairs (comma separated)\n" + > + "execsu [env pairs] [cmdline] - start program as privileged user\n" + can you add execcwdsu ?

(In reply to Joel Maher (:jmaher) from comment #27) > Comment on attachment 655607 [details] [diff] [review] > 4th and hopefully final take on the patch > > Review of attachment 655607 [details] [diff] [review]: > ----------------------------------------------------------------- > > a few logic issues in here and a few nits. > > ::: build/mobile/devicemanagerSUT.py > @@ +55,5 @@ > > if self.getDeviceRoot() == None: > > + raise BaseException("Failed to connect to SUT Agent and retrieve the device root.") > > + try: > > + verstring = self.runCmds([{ 'cmd': 'ver' }]) > > + self.agentVersion = re.sub('SUTAgentAndroid Version ', '', verstring) > > does this have any \n at the end or spaces we need to look out for? No. >>> import droid >>> dm = droid.DroidSUT('192.168.1.11') reconnecting socket >>> dm.agentVersion '1.11' >>> dm.runCmds([{ 'cmd': 'ver' }]) 'SUTAgentAndroid Version 1.11' > @@ +273,5 @@ > > cmdline = self._escapedCommandLine(cmd) > > if env: > > cmdline = '%s %s' % (self.formatEnvString(env), cmdline) > > > > + haveExecSu = (StrictVersion(self.agentVersion) >= StrictVersion('1.12')) > > 1.13, 1.12 is already deployed Oops. Will fix. > @@ +291,3 @@ > > try: > > if cwd: > > + self.sendCmds([{ 'cmd': '%s %s %s' % (cmd, cwd, cmdline) }], outputfile, timeout) > > what about execcwdsu? That should be limited to version 1.13 or higher. It is, if you look at the diff. The command is created by concatenating the string information, and 'su' will only be appended if we are running the required version. > @@ +294,3 @@ > > else: > > + if not root or haveExecSu: > > + self.sendCmds([{ 'cmd': '%s %s"' % (cmd, cmdline) }], outputfile, timeout) > > this will always execute if root=True, that won't work for backwards > compatibility. If my understanding of boolean logic and python is still correct, that's not true. :) >>> haveExecSu = False >>> root = True >>> not root or haveExecSu False I think maybe you're thinking that the not gets applied to the whole expression, when it only gets applied to the first one. We could rewrite it as "if (not root) or haveExecSu" if it makes things clearer for you. I normally hate unnecessary parens, but I think it might be ok in this one case. > ::: build/mobile/sutagent/android/DoCommand.java > @@ +3820,4 @@ > > "run [cmdline] - start program no wait\n" + > > "exec [env pairs] [cmdline] - start program no wait optionally pass env\n" + > > " key=value pairs (comma separated)\n" + > > + "execsu [env pairs] [cmdline] - start program as privileged user\n" + > > can you add execcwdsu ? Sure. I'll add execcwd at the same time.

Two key changes: * Change `if not root or haveExecSu` in dmSUT's shell to `if (not root) or haveExecSu` for clarity * Updated help in SUTAgent for execcwd and execcwdsu (had to reformat stuff so it would still fit)

Try run for 4b2f7621cf96 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=4b2f7621cf96 Results (out of 19 total builds): success: 17 warnings: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.com-4b2f7621cf96

Comment on attachment 655640 [details] [diff] [review] Fix issues pointed out by review Review of attachment 655640 [details] [diff] [review]: ----------------------------------------------------------------- ok, I am finally getting rid of the cobwebs that are causing me to misunderstand this, looks good. ::: build/mobile/devicemanagerSUT.py @@ +285,5 @@ > + cmd = "exec" > + if cwd: > + cmd += "cwd" > + if root and haveExecSu: > + cmd += "su" nit: 2 space indent to match the file.

Unfortunately I found some additional problems in testing this, in that quoted sections of the output (e.g. "foo bar baz") would get lost when run through su. This patch fixes that, along with the nits pointed out by jmaher last time. I also fixed some formatting goofs in the help.

Comment on attachment 656178 [details] [diff] [review] Additional fixes Review of attachment 656178 [details] [diff] [review]: ----------------------------------------------------------------- we just might get this landed! ::: build/mobile/devicemanagerSUT.py @@ +273,5 @@ > cmdline = self._escapedCommandLine(cmd) > if env: > cmdline = '%s %s' % (self.formatEnvString(env), cmdline) > > + haveExecSu = (StrictVersion(self.agentVersion) >= StrictVersion('1.12')) 1.13 ::: build/mobile/sutagent/android/DoCommand.java @@ +117,2 @@ > EXECCWD ("execcwd"), > + EXECCWDSU ("execcwd"), should this be execcwdsu?

Inbound with jmaher's fixes, plus a few more minor issues (stray quote, formatting problems) fixed: https://hg.mozilla.org/integration/mozilla-inbound/rev/dc5479d6e258

Renaming bug for clarity.

Pushed to mozbase as well: https://github.com/mozilla/mozbase/commit/e5c30fe84221a0e5b92a10b73299fc6d04e688e5