Re: Enable adb port in tramp adb

[Ashi]

The patch is updated based on your comments.

On Mon, Oct 13, 2014 at 12:28 AM, Michael Albinus <address@hidden> wrote:

Ashi <address@hidden> writes:

> Hi, all,

Hi Zhongwei,

> In this patch, I enabled the adb port access in tramp adb module. The
> patch is attached, please help review, thanks a lot!

Thanks for your contribution!

I have had a look on this. No real test, just code reading.

> +(defun tramp-adb-get-real-host (host)
> +  "Returns real host (with port or without port) from tramp host format
> +e.g. input: 192.168.1.1#5555 return 192.168.1.1:5555
> +     input: R38273882DE return R38273882DE"
> +  (let ((l-port)
> +        (l-host)
> +        (real-host))
> +    (if (string-match tramp-host-with-port-regexp host)
> +        (setq l-port (match-string 2 host)
> +              l-host (match-string 1 host)
> +              real-host (concat l-host ":" l-port))
> +      (setq real-host host))))
> +

Well, this function ought to convert a host name in tramp syntax into a
host name which shall be used by the adb command. First comment is, that
in Tramp we usually take VEC as argument for such kind of functions. By
this, you could apply all helper functions for parsing file name components.

Second remark is, that "*-real-host" function names in Tramp are used to
extract just the host name of a remote file name, without anything
else like the port. So this function name would be confusing.

Third (minor comment) is, that according to Emacs conventions the first
line of doc strings shall consist of a sentence with a final period. So
I would rewrite the function as (untested)

(defun tramp-adb-get-host-for-execution (vec)
  "Returns host name and port from VEC to be used in shell exceution.
E.g. a host name \"192.168.1.1#5555\" returns \"192.168.1.1:5555\"
     a host name \"R38273882DE\" returns \"R38273882DE\"."
  (if (tramp-file-name-port vec)
      (format
       "%s:%s" (tramp-file-name-real-host vec) (tramp-file-name-port vec))
    (tramp-file-name-host vec)))

 

Thanks, the code is much cleaner now!

>  (defun tramp-adb-execute-adb-command (vec &rest args)
>    "Returns nil on success error-output on failure."
> -  (when (> (length (tramp-file-name-host vec)) 0)
> -    (setq args (append (list "-s" (tramp-file-name-host vec)) args)))
> +  (let ((host (tramp-file-name-host vec)))
> +    (when (> (length host) 0)
> +      (setq args (append (list "-s" (tramp-adb-get-real-host host)) args))))
>    (with-temp-buffer

This shall be replaced by the call of (tramp-adb-get-host-for-execution vec)
However, since this is the only place this function is called, you could
use the code of the function directly.

> +    (let* ((host (tramp-file-name-host vec))
> +           (real-host (tramp-adb-get-real-host host)))

[...]

Well, I don't understand why this is needed. Since the devices variable
is just a temporary one, which keeps known adb targets, it doesn't
matter whether this is a list of ("192.168.1.1#5555" "R38273882DE")
or ("192.168.1.1:5555" "R38273882DE")

Maybe tramp-adb-parse-device-names must be adapted. Or do I miss something?

 

Yes, part of the problem is caused by  tramp-adb-parse-device-names.

And another one is the host name in "args" which is needed by adb command.

This patch is only tested by hand. I tried to run "make check", but there are many cases fails (26/35 fails) on current master. What can I do for this?

> Best regards,
> Zhongwei

Best regards, Michael.

0001-enable-adb-access-with-port-format-e.g.-adb-164.2.16.patch
Description: Text Data