1048942 - Add support for getting ip address to mozdevice.adb

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[James Graham [:jgraham]]

netcfg based support for getting the device ip address.

Comment 1

[James Graham [:jgraham]]

Attachment: Add support for getting the device ip address to mozdevice.adb

Comment 2

[Bob Clary [:bc] (inactive)]

Comment on attachment 8467847 [details] [diff] [review]
Add support for getting the device ip address to mozdevice.adb

Review of attachment 8467847 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if you are certain that the local interface will be the first line of output. If not, do you think that rejecting ip 127.0.0.1 is sufficient?

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +1029,5 @@
>  
> +    def get_ip_address(self, interfaces=None, timeout=None):
> +        """Returns the device's ip address, or None if it doesn't have one
> +
> +        :param interfaces: List of interfaces to allow, or None to alow any

alow -> allow

@@ +1045,5 @@
> +        """
> +        ip_regexp = re.compile(r'(\w+)\s+UP\s+([1-9]\d{0,2}\.\d{1,3}\.\d{1,3}\.\d{1,3})')
> +        data = self.shell_output('netcfg')
> +        lines = data.split("\n")
> +        for line in lines[1:]:

This is assuming the first line is always the lo interface? Is this guaranteed? Wouldn't it be safer to test for the interface below and continue if the ip is 127.0.0.1?

from https://android.googlesource.com/platform/system/core/+/master/netcfg/netcfg.c it seems the output of netcfg depends on the contents of the directory /sys/class/net. It isn't clear to me that the local interface's name is fixed nor that it is guaranteed to be the first line in the output.

maybe

for line in lines:

@@ +1048,5 @@
> +        lines = data.split("\n")
> +        for line in lines[1:]:
> +            match = ip_regexp.search(line)
> +            if match:
> +                interface, ip = match.groups()

maybe

if ip == '127.0.0.1':
    continue

Comment 3

[James Graham [:jgraham]]

Attachment: Add support for getting the device ip address to mozdevice.adb

Fixed up previous review comments.

Comment 4

[Bob Clary [:bc] (inactive)]

Comment on attachment 8469443 [details] [diff] [review]
Add support for getting the device ip address to mozdevice.adb

Review of attachment 8469443 [details] [diff] [review]:
-----------------------------------------------------------------

r+

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +1106,5 @@
> +            an ADBTimeoutError.
> +            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.
> +        :returns: string ip address of the device

I missed this before, but maybe add "or None if it could not be found" or something to that effect.

Comment 5

[James Graham [:jgraham]]

Attachment: Add support for getting the device ip address to mozdevice.adb

Updated with ContextManager patch removed.

Comment 6

[Bob Clary [:bc] (inactive)]

Comment on attachment 8472410 [details] [diff] [review]
Add support for getting the device ip address to mozdevice.adb

Review of attachment 8472410 [details] [diff] [review]:
-----------------------------------------------------------------

Please put the parts of this patch which are from bug 1048883 into that bug.

Comment 7

[James Graham [:jgraham]]

Attachment: Add support for getting the device ip address to mozdevice.adb

Comment 8

[James Graham [:jgraham]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca6515ffad9e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ca6515ffad9e