Closed
Bug 1048883
Opened 10 years ago
Closed 10 years ago
[mozdevice.adb] Add extra methods for pulling files, remounting and port forwarding
Categories
(Testing :: Mozbase, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: jgraham, Assigned: jgraham)
References
Details
Attachments
(2 files, 4 obsolete files)
These are needed for many use cases and are currently missing.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8467768 -
Flags: review?(bclary)
Comment 2•10 years ago
|
||
Comment on attachment 8467768 [details] [diff] [review] Add extra methods for remounting, pulling files and forwarding ports Review of attachment 8467768 [details] [diff] [review]: ----------------------------------------------------------------- Overall this is ok, but I'm going to r- so you can fix remount and make sure we are detecting errors if they occur. This will also give us a change to talk about the possibility of adding the additional methods I mentioned and give me another opportunity to review. ::: testing/mozbase/mozdevice/mozdevice/adb.py @@ +745,5 @@ > """ > return ADBCommand.command_output(self, cmds, > device_serial=self._device_serial, > timeout=timeout) > I think we should have an adbd_root method that calls adb -s serial root on the device. We can use it in remount below. We will probably need to check that adbd has completed restarting before returning. In addition, adb -s serial root has a 0 exit code regardless of whether it succeeds or fails. We should check the adbd process to see if it is running as root and if not raise an ADBError. You can get the adbd user from get_process_list... >>> from adb import ADBDevice >>> d = ADBDevice('b313b945') >>> p = d.get_process_list() >>> p[0] [1, '/init', 'root'] >>> a = [i for i in p if 'adbd' in i[1]] >>> a [[354, '/sbin/adbd', 'root']] >>> Perhaps it would be useful to have a method that returns the user that is running a process or at least checks if it is running as root. @@ +750,5 @@ > + # Port forwarding methods > + > + def forward(self, local, remote, allow_rebind=True, timeout=None): > + """Forward a local port to a specific port on the device. > + document the parameters and explain allow_rebind. do we want to validate the local and remote parameters to make sure they are a supported type? tcp:<port> localabstract:<unix domain socket name> localreserved:<unix domain socket name> localfilesystem:<unix domain socket name> dev:<character device name> jdwp:<process pid> (remote only) It wouldn't be too hard to check for leading (tcp|localabstract|localreserved|localfilesystem|dev|jdwp): @@ +755,5 @@ > + """ > + cmd = ["forward", local, remote] > + if not allow_rebind: > + cmd.insert(1, "--no-rebind") > + return self.command_output(cmd, timeout=timeout) No need to return the output is there? @@ +758,5 @@ > + cmd.insert(1, "--no-rebind") > + return self.command_output(cmd, timeout=timeout) > + > + def list_forwards(self, timeout=None): > + """Return a list of tuples specifying active forwards document the parameters. It actually returns a list of lists not a tuple of lists specifying the active forwards. Each list representing a forward is of the form ['device serial no', 'local', 'remote'] @@ +766,5 @@ > + return [line.split(" ") for line in forwards.split("\n") if line.strip()] > + > + def remove_forwards(self, local=None, timeout=None): > + """Return a list of tuples specifying active forwards > + document the parameters; docstring is wrong. @@ +773,5 @@ > + if local is not None: > + cmd.extend(["--remove", local]) > + else: > + cmd.extend(["--remove-all"]) > + return self.command_output(cmd, timeout=timeout) No need to return the output is there? @@ +1038,5 @@ > + This timeout is per adb call. The total time spent > + may exceed this value. If it is not specified, the value > + set in the ADBDevice constructor is used. > + :param root: optional boolean specifying if the command should > + be executed as root. There is no root parameter. @@ +1040,5 @@ > + set in the ADBDevice constructor is used. > + :param root: optional boolean specifying if the command should > + be executed as root. > + :raises: * ADBTimeoutError > + * ADBRootError Doesn't raise ADBRootError @@ +1041,5 @@ > + :param root: optional boolean specifying if the command should > + be executed as root. > + :raises: * ADBTimeoutError > + * ADBRootError > + * ADBError""" remount only works if adbd is running as root on the device, right? We should go ahead and make sure. self.adbd_root() @@ +1042,5 @@ > + be executed as root. > + :raises: * ADBTimeoutError > + * ADBRootError > + * ADBError""" > + return self.command_output(["remount"], timeout=timeout) No need to return output is there? We do need to check the output however. If the command succeeds, it returns 'remount succeeded' If the command fails, it returns 'remount failed: Operation not permitted' but the exit code is 0. We should check for 'remount succeeded'. @@ +1258,5 @@ > self.command_output(["push", os.path.realpath(local), remote], > timeout=timeout) > > + def pull(self, remote, local, timeout=None): > + """Pulls a file or directory from the device. should we support the -a option? @@ +1261,5 @@ > + def pull(self, remote, local, timeout=None): > + """Pulls a file or directory from the device. > + > + :param remote: string containing the name of the remote file or > + directory name. :param remote: string containing the path of the remote file or directory. @@ +1263,5 @@ > + > + :param remote: string containing the name of the remote file or > + directory name. > + :param local: string containing the name of the local file or > + directory name. :param local: string containing the path of the local file or directory. @@ +1281,4 @@ > def rm(self, path, recursive=False, force=False, timeout=None, root=False): > """Delete files or directories on the device. > > :param path: string containing the file name on the device. You could fix this to match my recommendations above.... :param path: string containing the path on the device to be removed.
Attachment #8467768 -
Flags: review?(bclary) → review-
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Bob Clary [:bc:] from comment #2) > I think we should have an adbd_root method that calls adb -s serial root on > the device. We can use it in remount below. We will probably need to check > that adbd has completed restarting before returning. > > In addition, adb -s serial root has a 0 exit code regardless of whether it > succeeds or fails. We should check the adbd process to see if it is running > as root and if not raise an ADBError. You can get the adbd user from > get_process_list... (fwiw "adb shell id" seems like a more pure way of getting the userid of adbd, but I guess we already have parsing for the ps output). So I don't think I can do this, because I can't get adbd running as non-root on a rooted B2G device (or-vice-versa). The code I wrote is: def ensure_root(self): def _is_root(): processes = self.get_process_list() is_root = False for pid, cmd, user in processes: if cmd.enswith("/adbd"): is_root = user == "root" break return _is_root if not is_root(): self.command_output(["root"]) # Not sure how to pause until this has actually restarted here if not _is_root(): raise ADBError("Failed to restart adbd as root") But I can't test it at all.
Assignee | ||
Comment 4•10 years ago
|
||
Added most of the requested changes except the stuff to put adbd in root mode since I don't have any way to test that. If you want it as part of this patch can you add it? Otherwise we could defer to a later bug.
Attachment #8469346 -
Flags: feedback?(bclary)
Comment 5•10 years ago
|
||
(In reply to James Graham [:jgraham] from comment #3) > (In reply to Bob Clary [:bc:] from comment #2) > > > I think we should have an adbd_root method that calls adb -s serial root on > > the device. We can use it in remount below. We will probably need to check > > that adbd has completed restarting before returning. > > > > In addition, adb -s serial root has a 0 exit code regardless of whether it > > succeeds or fails. We should check the adbd process to see if it is running > > as root and if not raise an ADBError. You can get the adbd user from > > get_process_list... > > (fwiw "adb shell id" seems like a more pure way of getting the userid of > adbd, but I guess we already have parsing for the ps output). > That does sound better and is supported in both android and b2g and doesn't have to slog through the process list each time. Let's do that. A new ADBDevice shell id command? > So I don't think I can do this, because I can't get adbd running as non-root > on a rooted B2G device (or-vice-versa). The code I wrote is: > Not sure what you mean. If it is already running as root, then we are fine. We could check prior to calling root and not do the adb root at all, but I wouldn't consider it an error to call root on a device already running adbd as root. > def ensure_root(self): > def _is_root(): > processes = self.get_process_list() > is_root = False > > for pid, cmd, user in processes: > if cmd.enswith("/adbd"): > is_root = user == "root" > break > return _is_root > > if not is_root(): > self.command_output(["root"]) > # Not sure how to pause until this has actually restarted here all of our commands have a wait-for-device prepended. Perhaps we don't need to, or we can poll until the id returns root again. > if not _is_root(): > raise ADBError("Failed to restart adbd as root") > > But I can't test it at all. (In reply to James Graham [:jgraham] from comment #4) > Created attachment 8469346 [details] [diff] [review] > Add extra methods for remounting, pulling files and forwarding ports > > Added most of the requested changes except the stuff to put adbd in root mode > since I don't have any way to test that. If you want it as part of this > patch can > you add it? Otherwise we could defer to a later bug. Sure, let me think about it while reviewing. Thanks!
Comment 6•10 years ago
|
||
Comment on attachment 8469346 [details] [diff] [review] Add extra methods for remounting, pulling files and forwarding ports Review of attachment 8469346 [details] [diff] [review]: ----------------------------------------------------------------- f+. We can worry about adb root later. ::: testing/mozbase/mozdevice/mozdevice/adb.py @@ +790,5 @@ > + for port, prefixes in [(local, prefix_local), > + (remote, prefix_remote)]: > + parts = port.split(":", 1) > + if len(parts) != 2 or parts[0] not in prefixes: > + raise ValueError("Invalid forward specifier %s" % port) Can we make the port value checking into a helper function that could be used here as well as below? def _validate_forward_port(port, local_only=False): ? @@ +816,5 @@ > + > + def remove_forwards(self, local=None, timeout=None): > + """Remove existing port forwards. > + > + :param local: local port specifier as for ADBDevice.forward :param local: local port specifier as for ADBDevice.forward. If local is not specified, removes all forwards. @@ +831,5 @@ > + if local is not None: > + prefixes = ["tcp", "localabstract", "localreserved", "localfilesystem", "dev"] > + parts = port.local(":", 1) > + if len(parts) != 2 or parts[0] not in prefixes: > + raise ValueError("Invalid forward specifier %s" % port) use helper here? _validate_forward_port(parts[0], local_only=True) @@ +835,5 @@ > + raise ValueError("Invalid forward specifier %s" % port) > + > + cmd.extend(["--remove", local]) > + else: > + cmd.extend(["--remove-all"]) I forget who influenced me, maybe Kernighan and Plauger, but I'd prefer the if look like: if local is None: cmd.extend(["--remove-all"]) else: ....
Attachment #8469346 -
Flags: feedback?(bclary) → feedback+
Assignee | ||
Comment 7•10 years ago
|
||
Fixed up with previously mentioned changes.
Attachment #8467768 -
Attachment is obsolete: true
Attachment #8469346 -
Attachment is obsolete: true
Attachment #8469439 -
Flags: review?(bclary)
Comment 8•10 years ago
|
||
Comment on attachment 8469439 [details] [diff] [review] Add extra methods for remounting, pulling files and forwarding ports Review of attachment 8469439 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mozbase/mozdevice/mozdevice/adb.py @@ +763,5 @@ > + if is_local: > + prefixes = prefixes_local > + else: > + prefixes = prefixes_remote > + we could write this as: prefixes = ["tcp", "localabstract", "localreserved", "localfilesystem", "dev"] if not is_local: prefixes += ["jdwp"] It's up to you.
Attachment #8469439 -
Flags: review?(bclary) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Updated with ContextManager patch removed.
Assignee | ||
Updated•10 years ago
|
Attachment #8469439 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8472409 -
Flags: review?(bclary)
Comment 10•10 years ago
|
||
Comment on attachment 8472409 [details] [diff] [review] Add extra methods for remounting, pulling files and forwarding ports Review of attachment 8472409 [details] [diff] [review]: ----------------------------------------------------------------- carrying forward r+
Attachment #8472409 -
Flags: review?(bclary) → review+
Comment 11•10 years ago
|
||
james, you mixed part of updates for the review comments from this bug into bug 1048942's patch. Please update both patches so they don't bleed into each other.
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8472409 -
Attachment is obsolete: true
Attachment #8473055 -
Flags: review?(bclary)
Comment 13•10 years ago
|
||
Comment on attachment 8473055 [details] [diff] [review] Add extra methods for remounting, pulling files and forwarding ports Review of attachment 8473055 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mozbase/mozdevice/mozdevice/adb.py @@ +790,5 @@ > + * ADBTimeoutError > + * ADBError > + """ > + > + prefix_local = ["tcp", "localabstract", "localreserved", "localfilesystem", "dev"] not used any more. delete this and the extra blank lines.
Attachment #8473055 -
Flags: review?(bclary) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Carrying forward r+
Assignee | ||
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e96ce6c8bb25
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e96ce6c8bb25
Assignee: nobody → james
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•