|Subject:||Re: Enable adb port in tramp adb|
|Date:||Mon, 20 Oct 2014 18:28:58 +0800|
Ashi <address@hidden> writes:
> Hi, all,
> 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)
"%s:%s" (tramp-file-name-real-host vec) (tramp-file-name-port vec))
> (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))))
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?
> Best regards,
Best regards, Michael.
Description: Text Data
|[Prev in Thread]||Current Thread||[Next in Thread]|