search

ARM-software / devlib

Library for interaction with and instrumentation of remote devices.
Apache License 2.0
47 stars 78 forks source link

utils/android: Fix adb_root() exceptions #617

Closed douglas-raillard-arm closed 1 year ago

douglas-raillard-arm commented 1 year ago

Ensure adb_root() always raises AdbRootError so that the caller can catch it reliably. This is especially important since adb_root() failing is ignored and simply triggers a fallback on using su. Android production builds refuse adb root nowadays, so it's important that adb root failures are handled well.

douglas-raillard-arm commented 1 year ago

Currently, this leads to the first Target() creation to fail on android production build, then the 2nd Target succeeds

This is because we still register the previous as an active connection (__init__ did not finish successfully and is therefore not counted as initialized, therefore _close() is not run by __del__ and active_connection is not decremented). When an active connection is detected, we skip adb_root() altogether (as this implies an adbd restart), and things "work".

This PR fixes the flow so that adb_root() failure is handled in the first place, so that the first Target() call succeeds by falling back on su.

marcbonnici commented 1 year ago

Just thinking out loud here, but could it make more sense to fix the consistency issue you mentioned, so that subsequent requets also fail, rather than to enable this fallback method?

I'm just wondeing if there could be a reason a user has explictally requested adb_root to be enabled vs devlib decided to fall back to using su under the hood, and whether this could cause any confusion?

douglas-raillard-arm commented 1 year ago

I think adb root is quite unreliable but at the same time might be the only way to become root if the device does not have proper su support, which AFAIR happened in the past. So that means the world is divided between devices with working su and forbidden adb root, or no su and working adb root.

The current situation with the fallback allows for a reliable configuration to provide root whenever target.execute(as_root=True) is used, which is all what is required really for things to work. IMHO forcing the user into faffing around with some settings depending on the device is just going to deteriorate the UX.