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)

x86_64
Linux
defect
Not set
normal

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.
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-
(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.
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)
(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 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+
Fixed up with previously mentioned changes.
Attachment #8467768 - Attachment is obsolete: true
Attachment #8469346 - Attachment is obsolete: true
Attachment #8469439 - Flags: review?(bclary)
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+
Updated with ContextManager patch removed.
Attachment #8469439 - Attachment is obsolete: true
Attachment #8472409 - Flags: review?(bclary)
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+
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.
Attachment #8472409 - Attachment is obsolete: true
Attachment #8473055 - Flags: review?(bclary)
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+
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.

Attachment

General

Created:
Updated:
Size: