Re: master babe6a5e948 1/2: Introduce a new TRAMP method `androidsu'

[Michael Albinus <michael.albinus@gmx.de>]

Po Lu <luangruo@yahoo.com> writes:

Hi,

>> FWIW, after a very very first look on your code I'm even not convinced
>> we need a new Tramp backend for what you want to achieve.
>
> Perhaps not, but at the very minimum a replacement for:
>
>   ;; Android su binaries contact a background service to
>   ;; obtain authentication; during this process, input
>   ;; received is discarded, so input cannot be
>   ;; guaranteed to reach the root shell until its prompt
>   ;; is displayed.
>   (with-current-buffer (process-buffer p)
>     (tramp-wait-for-regexp p tramp-connection-timeout
>                            "#[[:space:]]*$"))
>
> will be required during connection setup.  Thanks.

Well, I've now took the time to review tramp-androidsu.el. As said I'm
nervous about using two Tramp backends in parallel. They have different
strategies, for example how they establish a connection, how they
handle the prompt etc pp, and so you have different hacks for this.

tramp-sh.el and tramp-adb.el have their own life. There might be
internal changes w/o any notice which would let androidsu.el fail. So if
we go this way, we must be very careful *not* to depend on their
internals too much.

Tramp knows already how to implement a given handler for different
backends. All these handlers which are used for more than one backend
are called tramp-handle-FUNCTION-NAME, and they are located in
tramp.el. Perhaps we could use this approach also here in more cases
than now, factoring out code

Btw, all what I propose in the following is just based on code reading. I
have no chance to test it. Does there exist an emulation, where I could
run Emacs in an Android environment? Note that I'm a n00b wrt Android
development. tramp-adb.el was contributed by somebody else, and I test
it on my Samsung Galaxy S6 and Pixel 6.

Here's my review:

--8<---------------cut here---------------start------------->8---
;;; tramp-androidsu.el --- TRAMP method for Android superuser shells  -*- lexical-binding:t -*-
--8<---------------cut here---------------end--------------->8---

Tramp is spelled out "Tramp". See the Tramp manual, FAQ chapter.

--8<---------------cut here---------------start------------->8---
;; Keywords: comm, processes
;; Package: tramp
--8<---------------cut here---------------end--------------->8---

The Author: is missing.

--8<---------------cut here---------------start------------->8---
;;; Commentary:

;; The `su' method struggles (as do other shell-based methods) with the
;; crippled versions of many Unix utilities installed on Android,
;; workarounds for which are implemented in the `adb' method.  This
;; method defines a shell-based method that is identical in function to
;; `su', but reuses such code from the `adb' method where applicable and
;; also provides for certain mannerisms of popular Android `su'
;; implementations.
--8<---------------cut here---------------end--------------->8---

It should say explicitly, that this is the "su" implementation for Emacs
running on `android' systems.

--8<---------------cut here---------------start------------->8---
;;;###tramp-autoload
(defconst tramp-androidsu-method "androidsu"
  "When this method name is used, forward all calls to su.")
--8<---------------cut here---------------end--------------->8---

This is not very convenient. Why not using the method "su"? It is
obvious, that on `android' systems `tramp-androidsu-file-name-handler'
is used, and `tramp-sh-file-name-handler' otherwise. We have already a
similar constellation: the "smb" method is implemented in both
tramp-smb.el and tramp-gvfs.el.

--8<---------------cut here---------------start------------->8---
;;;###tramp-autoload
(defcustom tramp-androidsu-mount-global-namespace t
  "When non-nil, browse files from within the global mount namespace.
On systems that assign each application a unique view of the filesystem
by executing them within individual mount namespaces and thus conceal
each application's data directories from others, invoke `su' with the
option `-mm' in order for the shell launched to run within the global
mount namespace, so that TRAMP may edit files belonging to any and all
applications."
  :group 'tramp
  :version "30.1"
  :type 'boolean)
--8<---------------cut here---------------end--------------->8---

Well, this sounds very internal. I, as a user, wouldn't know whether I
shall customize it to t or nil. Couldn't it be auto-detected?

--8<---------------cut here---------------start------------->8---
(defvar tramp-androidsu-su-mm-supported 'unknown
  "Whether `su -mm' is supported on this system.")
--8<---------------cut here---------------end--------------->8---

Looks to me as candidate for a connection property.

--8<---------------cut here---------------start------------->8---
;;;###tramp-autoload
(tramp--with-startup
 (add-to-list 'tramp-methods
	      `(,tramp-androidsu-method
                (tramp-login-program        "su")
                (tramp-login-args           (("-") ("%u")))
                (tramp-remote-shell         "/system/bin/sh")
                (tramp-remote-shell-login   ("-l"))
                (tramp-remote-shell-args    ("-c"))
                (tramp-tmpdir               "/data/local/tmp")
                (tramp-connection-timeout   10)))
--8<---------------cut here---------------end--------------->8---

Should `tramp-remote-shell' and `tramp-tmpdir' be hard coded? I know
that the respective strings are hard coded in tramp-adb.el, but now
seems to be time to use them via a defconst.

--8<---------------cut here---------------start------------->8---
 (add-to-list 'tramp-default-host-alist
              `(,tramp-androidsu-method nil "localhost"))
--8<---------------cut here---------------end--------------->8---

I would also add

 (add-to-list 'tramp-default-user-alist
	      `(,tramp-androidsu-method nil ,tramp-root-id-string))

See also tramp-sh.el.

--8<---------------cut here---------------start------------->8---
		     (p (start-process (tramp-get-connection-name vec)
			               (tramp-get-connection-buffer vec)
                                       ;; Disregard
                                       ;; tramp-encoding-shell, as
                                       ;; there's no guarantee that it's
                                       ;; possible to execute it with
                                       ;; `android-use-exec-loader' off.
			               "/system/bin/sh" "-i"))
--8<---------------cut here---------------end--------------->8---

Pls use the constant for the shell name.

--8<---------------cut here---------------start------------->8---
		(setq command (format "exec su - %s || exit"
				      (or user "root")))
--8<---------------cut here---------------end--------------->8---

If you have set `tramp-default-user-alist', `user' has always the
proper value.

--8<---------------cut here---------------start------------->8---
		(tramp-adb-send-command vec command t t)
		;; Android su binaries contact a background service to
		;; obtain authentication; during this process, input
		;; received is discarded, so input cannot be
		;; guaranteed to reach the root shell until its prompt
		;; is displayed.
		(with-current-buffer (process-buffer p)
		  (tramp-wait-for-regexp p tramp-connection-timeout
					 "#[[:space:]]*$"))
--8<---------------cut here---------------end--------------->8---

Why not (tramp-adb-wait-for-output p tramp-connection-timeout) ?
It knows the adb shell prompt, and it uses the proper buffer (no need
for (with-current-buffer ...)

--8<---------------cut here---------------start------------->8---
                ;; Disable Unicode.
                (tramp-adb-send-command vec "set +U")
--8<---------------cut here---------------end--------------->8---

Why? And is it guaranteed, that all shells residing on Android devices
know that option?

--8<---------------cut here---------------start------------->8---
		;; Set the remote PATH to a suitable value.
		(tramp-set-connection-property vec "remote-path"
					       '("/system/bin"
                                                 "/system/xbin"))
--8<---------------cut here---------------end--------------->8---

Please don't hard code. Make it configurable.

--8<---------------cut here---------------start------------->8---
(defun tramp-androidsu-generate-wrapper (function)
  "Return connection wrapper function for FUNCTION.
Return a function which temporarily substitutes local replacements for
the `adb' method's connection management functions around a call to
FUNCTION."
  (lambda (&rest args)
    (let ((tramp-adb-wait-for-output
           (symbol-function #'tramp-adb-wait-for-output))
          (tramp-adb-maybe-open-connection
           (symbol-function #'tramp-adb-maybe-open-connection)))
      (unwind-protect
          (progn
            ;; tramp-adb-wait-for-output addresses problems introduced
            ;; by the adb utility itself, not Android utilities, so
            ;; replace it with the regular TRAMP function.
            (fset 'tramp-adb-wait-for-output #'tramp-wait-for-output)
            ;; Likewise, except some special treatment is necessary on
            ;; account of flaws in Android's su implementation.
            (fset 'tramp-adb-maybe-open-connection
                  #'tramp-androidsu-maybe-open-connection)
            (apply function args))
        ;; Restore the original definitions of the functions overridden
        ;; above.
        (fset 'tramp-adb-wait-for-output tramp-adb-wait-for-output)
        (fset 'tramp-adb-maybe-open-connection tramp-adb-maybe-open-connection)))))
--8<---------------cut here---------------end--------------->8---

I really don't know whether this is a good idea. We have basic file
functions which need to handle more than one file. If you have for
example '(copy-file SOURCE TARGET)', and SOURCE uses the androidsu
method, then `tramp-androidsu-sh-handle-copy-file' will be called. It
does everything for SOURCE, but it needs also to do some basic file
operations for TARGET. And TARGET could be a remote file name with
*another* Tramp method, which could fail then due to your hack.

--8<---------------cut here---------------start------------->8---
(defalias 'tramp-androidsu-handle-access-file
  (tramp-androidsu-generate-wrapper #'tramp-handle-access-file))
--8<---------------cut here---------------end--------------->8---

All handlers from tramp.el are called `tramp-handle-FUNCTION-NAME', and
they work w/o any change. Please don't declare wrapper funtions for
them, and use `tramp-handle-FUNCTION-NAME' in
`tramp-androidsu-file-name-handler-alist'.

--8<---------------cut here---------------start------------->8---
(defalias 'tramp-androidsu-sh-handle-copy-file
  (tramp-androidsu-generate-wrapper #'tramp-sh-handle-copy-file))

(defalias 'tramp-androidsu-adb-handle-delete-directory
  (tramp-androidsu-generate-wrapper #'tramp-adb-handle-delete-directory))
--8<---------------cut here---------------end--------------->8---

Please don't use implementation details (for example, which backend a
given function is derived from), in its name. Such wrappers shall be
called `tramp-androidsu-handle-copy-file' and
`tramp-androidsu-handle-delete-directory'.

--8<---------------cut here---------------start------------->8---
	  (setq
	   p (make-process
	      :name name :buffer buffer
	      :command (if (tramp-get-connection-property v "remote-namespace")
                           (append (list "su" "-mm" "-" (or user "root") "-c")
                                   command)
                         (append (list "su" "-" (or user "root") "-c")
                                 command))
	      :coding coding :noquery noquery :connection-type connection-type
	      :sentinel sentinel :stderr stderr))
--8<---------------cut here---------------end--------------->8---

`user' has already a proper value. Don't hard-code "root".

Just a remark for your documentation change in tramp.texi. You have
added some few words in the "Quick Start Guide" node, which is OK. But
it should be in the "3.3 Using ‘su’, ‘sudo’, ‘doas’ and ‘sg’" or "3.10
Using Android" section, and not in "3.4 Combining ‘ssh’ or ‘plink’ with
‘su’, ‘sudo’ or ‘doas’". And don't mention multi-hops here.

The more verbose description shall be added in node "Inline methods".

That's it for the moment.

Best regards, Michael.