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

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

[Michael Albinus]

Po Lu via Mailing list for Emacs changes <emacs-diffs@gnu.org> writes:

Hi,

> branch: master
> commit babe6a5e948985f961ffd36f64323950abd98b7f
> Author: Po Lu <luangruo@yahoo.com>
> Commit: Po Lu <luangruo@yahoo.com>
>
>     Introduce a new TRAMP method `androidsu'

You are aware that Tramp is a package on its own, maintained outside the
Emacs repository? I would have appreciated if you had contacted the
maintainer prior pushing such substantial changes. And you make it hard
to decide, whether your (architectural) approach is best suited, now
that it has landed already in Emacs master. At least pushing to a
separate branch would be required. Even better, if this process happens
in the Tramp repository.

I will review your code. Note that it might take a while due to my
restricted availability.

Michael.

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

[Po Lu]

Michael Albinus <michael.albinus@gmx.de> writes:

> You are aware that Tramp is a package on its own, maintained outside the
> Emacs repository?

I was under the impression that maintainers of such packages are
expected to merge changes applied to Emacs at whatever frequency suits
them.  CC Mode and Org Mode both observe this protocol, for what it's
worth.

> I would have appreciated if you had contacted the maintainer prior
> pushing such substantial changes.  And you make it hard to decide,
> whether your (architectural) approach is best suited, now that it has
> landed already in Emacs master.  At least pushing to a separate branch
> would be required.  Even better, if this process happens in the Tramp
> repository.

OK, sorry.  However, we have until Emacs 30.1 to decide whether this
feature is ideal for Emacs, so there's no shortage of time.

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

[Michael Albinus]

Po Lu <luangruo@yahoo.com> writes:

>> You are aware that Tramp is a package on its own, maintained outside the
>> Emacs repository?
>
> I was under the impression that maintainers of such packages are
> expected to merge changes applied to Emacs at whatever frequency suits
> them.  CC Mode and Org Mode both observe this protocol, for what it's
> worth.

And so does Tramp. The problem isn't the frequency, but the amount of
changes you have applied w/o synchronization.

>> I would have appreciated if you had contacted the maintainer prior
>> pushing such substantial changes.  And you make it hard to decide,
>> whether your (architectural) approach is best suited, now that it has
>> landed already in Emacs master.  At least pushing to a separate branch
>> would be required.  Even better, if this process happens in the Tramp
>> repository.
>
> OK, sorry.  However, we have until Emacs 30.1 to decide whether this
> feature is ideal for Emacs, so there's no shortage of time.

No shortage of time. But restrictions in what we can change afterwards.

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.

Michael.

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

[Po Lu]

Michael Albinus <michael.albinus@gmx.de> writes:

> 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.

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

[Michael Albinus]

Po Lu <luangruo@yahoo.com> writes:

Hi,

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> 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.

I will check the majority of the changes over the weekend. I'll be OOO
next two days.

For now, I've reviewed your changes in tramp-adb.el. In general they are
OK, except the check for tramp-androidsu-method in tramp-adb-send-command.
It doesn't make sense (we must special case also tramp-adb-method), so
I've reverted this. In general, even if we're going with your approach,
we must NOT produce spaghetti code. Using tramp-adb objects in
tramp-androidsu might be OK, but then not the other direction.

Best regards, Michael.

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

[Michael Albinus]

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.

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

[Po Lu]

Michael Albinus <michael.albinus@gmx.de> writes:

> 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.

Thanks.  I'll try to address as many of your concerns as possible.

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

Okay.

> The Author: is missing.

I'd prefer not to leave my e-mail address in this file, subject to
change as it is.

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

Okay.

> 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.

That's fine by me, but please explain how to implement this or direct me
to documentation with this information, because I did not find any that
covered writing new methods.

> ;;;###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)
>
>
> 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?

Unfortunately not, since the correct value varies by the directories to
be accessed: with the option off, external storage devices are mounted
as perceived by Emacs but private application and system data is not
accessible, while otherwise this data is visible but potentially not
those storage devices.

> (defvar tramp-androidsu-su-mm-supported 'unknown
>   "Whether `su -mm' is supported on this system.")
>
>
> Looks to me as candidate for a connection property.

How so?  From what I understand, connection properties apply to either
complete connection lists or connection processes, while the presence of
an option provided by `su' is unaffected by any connection parameters
except the method itself that influence a connection's identify.

> ;;;###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)))
>
>
> 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.
>
>  (add-to-list 'tramp-default-host-alist
>               `(,tramp-androidsu-method nil "localhost"))
>
>
> I would also add
>
>  (add-to-list 'tramp-default-user-alist
> 	      `(,tramp-androidsu-method nil ,tramp-root-id-string))
>
> See also tramp-sh.el.
>
> 		     (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"))
>
>
> Pls use the constant for the shell name.

Where's the harm in this, though?  The location of /system/bin/sh will
never change, so there will never be reason for either users or
developers to replace it by anything else.

> 		(setq command (format "exec su - %s || exit"
> 				      (or user "root")))
>
>
> If you have set `tramp-default-user-alist', `user' has always the
> proper value.

I see, thanks.

> 		(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:]]*$"))
>
>
> 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 ...)

tramp-adb-wait-for-output also attempts to detect and delete certain
character sequences printed by adb, which is inappropriate when the
shell is being directly run in an Android device.

>                 ;; Disable Unicode.
>                 (tramp-adb-send-command vec "set +U")
>
> Why?

File names with Unicode characters outside the BMP cannot be opened
otherwise.

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

I don't think Android shells on the oldest of systems support this
option, but what's the worst that could happen, aside from an error
message's being printed and discarded?

> 		;; Set the remote PATH to a suitable value.
> 		(tramp-set-connection-property vec "remote-path"
> 					       '("/system/bin"
>                                                  "/system/xbin"))
>
>
> Please don't hard code. Make it configurable.

OK.

> (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)))))
>
>
> 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.

This won't be a problem, since this wrapper only overrides functions
defined in tramp-adb.el, which I don't anticipate anyone using on
Android.

> (defalias 'tramp-androidsu-handle-access-file
>   (tramp-androidsu-generate-wrapper #'tramp-handle-access-file))
>
>
> 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'.
>
> (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))
>
>
> 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'.
>
> 	  (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))
>
> `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.

Okay, thanks.

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

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

Po Lu <luangruo@yahoo.com> writes:

Hi,

>> 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.

Do you have a recommendation how I could use an emulator?

>> 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.
>
> That's fine by me, but please explain how to implement this or direct me
> to documentation with this information, because I did not find any that
> covered writing new methods.

It's not documented :-(

I cannot test myself, but the appended patch might do. Could you, pls, check?

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1443 bytes --]

diff --git a/lisp/net/tramp-androidsu.el b/lisp/net/tramp-androidsu.el
index 1623a0341b2..8704d22e00c 100644
--- a/lisp/net/tramp-androidsu.el
+++ b/lisp/net/tramp-androidsu.el
@@ -37,7 +37,7 @@
 (require 'tramp-sh)

 ;;;###tramp-autoload
-(defconst tramp-androidsu-method "androidsu"
+(defconst tramp-androidsu-method "su"
   "When this method name is used, forward all calls to su.")

 ;;;###tramp-autoload
@@ -66,10 +66,7 @@ tramp-androidsu-su-mm-supported
                 (tramp-remote-shell-login   ("-l"))
                 (tramp-remote-shell-args    ("-c"))
                 (tramp-tmpdir               "/data/local/tmp")
-                (tramp-connection-timeout   10)))
-
- (add-to-list 'tramp-default-host-alist
-              `(,tramp-androidsu-method nil "localhost")))
+                (tramp-connection-timeout   10))))

 (defvar android-use-exec-loader) ; androidfns.c.

@@ -645,7 +642,8 @@ tramp-androidsu-file-name-handler-alist
 (defsubst tramp-androidsu-file-name-p (vec-or-filename)
   "Check whether VEC-OR-FILENAME is for the `androidsu' method."
   (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
-    (equal (tramp-file-name-method vec) tramp-androidsu-method)))
+    (and (eq system-type 'android)
+         (equal (tramp-file-name-method vec) tramp-androidsu-method))))

 ;;;###tramp-autoload
 (defun tramp-androidsu-file-name-handler (operation &rest args)

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

[Po Lu]

Michael Albinus <michael.albinus@gmx.de> writes:

> Do you have a recommendation how I could use an emulator?

Yes, the android-x86 project provides rooted builds of Android
installable in the same manner as any other PC operating system:

  https://sourceforge.net/projects/android-x86/

> It's not documented :-(
>
> I cannot test myself, but the appended patch might do. Could you, pls,
> check?

I will in a little while, thanks.

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

[Michael Albinus]

Po Lu <luangruo@yahoo.com> writes:

Hi,

>> Do you have a recommendation how I could use an emulator?
>
> Yes, the android-x86 project provides rooted builds of Android
> installable in the same manner as any other PC operating system:
>
>   https://sourceforge.net/projects/android-x86/

Thanks, will try.

Best regards, Michael.

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

[Po Lu]

Michael Albinus <michael.albinus@gmx.de> writes:

> -(defconst tramp-androidsu-method "androidsu"
> +(defconst tramp-androidsu-method "su"
>    "When this method name is used, forward all calls to su.")
>
>  ;;;###tramp-autoload
> @@ -66,10 +66,7 @@ tramp-androidsu-su-mm-supported
>                  (tramp-remote-shell-login   ("-l"))
>                  (tramp-remote-shell-args    ("-c"))
>                  (tramp-tmpdir               "/data/local/tmp")
> -                (tramp-connection-timeout   10)))
> -
> - (add-to-list 'tramp-default-host-alist
> -              `(,tramp-androidsu-method nil "localhost")))
> +                (tramp-connection-timeout   10))))
>
>  (defvar android-use-exec-loader) ; androidfns.c.
>
> @@ -645,7 +642,8 @@ tramp-androidsu-file-name-handler-alist
>  (defsubst tramp-androidsu-file-name-p (vec-or-filename)
>    "Check whether VEC-OR-FILENAME is for the `androidsu' method."
>    (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
> -    (equal (tramp-file-name-method vec) tramp-androidsu-method)))
> +    (and (eq system-type 'android)
> +         (equal (tramp-file-name-method vec) tramp-androidsu-method))))
>
>  ;;;###tramp-autoload
>  (defun tramp-androidsu-file-name-handler (operation &rest args)

I've yet to test this patch, but nevertheless won't it override the
default file name handlers even in multi-hop connections, where su will
definitely not be executed under Android?

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

[Michael Albinus]

Po Lu <luangruo@yahoo.com> writes:

Hi,

> I've yet to test this patch, but nevertheless won't it override the
> default file name handlers even in multi-hop connections, where su will
> definitely not be executed under Android?

No. In multi-hop connections, every hop is checked via
tramp-multi-hop-p. This includes tramp-sh-file-name-handler-p, which
returns nil for the tramp-androidsu backend.

Best regards, Michael.

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

[Po Lu]

Michael Albinus <michael.albinus@gmx.de> writes:

> No. In multi-hop connections, every hop is checked via
> tramp-multi-hop-p. This includes tramp-sh-file-name-handler-p, which
> returns nil for the tramp-androidsu backend.

That raises a different question: won't such a check disable multi-hop
connections involving the `su' method from being established to
non-Android hosts?  Is it possible to detect whether VEC appertains to a
multi-hop connection early, in tramp-androidsh-file-name-handler-p
itself?

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

[Michael Albinus]

Po Lu <luangruo@yahoo.com> writes:

Hi,

>> No. In multi-hop connections, every hop is checked via
>> tramp-multi-hop-p. This includes tramp-sh-file-name-handler-p, which
>> returns nil for the tramp-androidsu backend.
>
> That raises a different question: won't such a check disable multi-hop
> connections involving the `su' method from being established to
> non-Android hosts?  Is it possible to detect whether VEC appertains to a
> multi-hop connection early, in tramp-androidsh-file-name-handler-p
> itself?

Good question. So perhaps we must improve the check like (untested)

(defsubst tramp-androidsu-file-name-p (vec-or-filename)
  "Check whether VEC-OR-FILENAME is for the `androidsu' method."
  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
    (and (eq system-type 'android)
         (equal (tramp-file-name-method vec) tramp-androidsu-method)
         (null (tramp-file-name-hop vec)))))

Best regards, Michael.

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

[Po Lu]

Michael Albinus <michael.albinus@gmx.de> writes:

> Good question. So perhaps we must improve the check like (untested)
>
> (defsubst tramp-androidsu-file-name-p (vec-or-filename)
>   "Check whether VEC-OR-FILENAME is for the `androidsu' method."
>   (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
>     (and (eq system-type 'android)
>          (equal (tramp-file-name-method vec) tramp-androidsu-method)
>          (null (tramp-file-name-hop vec)))))
>
> Best regards, Michael.

Too bad the two methods still cannot share the same name, as in that
case their respective entries in tramp-methods cannot coexist.

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

[Michael Albinus]

Po Lu <luangruo@yahoo.com> writes:

Hi,

>> Good question. So perhaps we must improve the check like (untested)
>>
>> (defsubst tramp-androidsu-file-name-p (vec-or-filename)
>>   "Check whether VEC-OR-FILENAME is for the `androidsu' method."
>>   (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
>>     (and (eq system-type 'android)
>>          (equal (tramp-file-name-method vec) tramp-androidsu-method)
>>          (null (tramp-file-name-hop vec)))))
>
> Too bad the two methods still cannot share the same name, as in that
> case their respective entries in tramp-methods cannot coexist.

Hmm. Perhaps this could be fixed by using special `tramp-androidsu-*'
parameters in the "su" entry of `tramp-methods'. But this might go to
far; I let it for you whether you want to try it.

Best regards, Michael.

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

[Michael Albinus]

Po Lu <luangruo@yahoo.com> writes:

Hi,

>> The Author: is missing.
>
> I'd prefer not to leave my e-mail address in this file, subject to
> change as it is.

Then use "Author: Po Lu", w/o an address. It is better than nothing.

>> (defvar tramp-androidsu-su-mm-supported 'unknown
>>   "Whether `su -mm' is supported on this system.")
>>
>> Looks to me as candidate for a connection property.
>
> How so?  From what I understand, connection properties apply to either
> complete connection lists or connection processes, while the presence of
> an option provided by `su' is unaffected by any connection parameters
> except the method itself that influence a connection's identify.

Connection properties are always good for a whole tramp-file-name
structure w/o localname and hop. So you could do

(with-tramp-connection-property vec "mm-supported"
  <code to check whether it is supported>)

The first time it is used, the <code ...> is evaluated, cached, and
returned. Any further call of this construct returns the cache value.

If you use VEC, the result is cached persistently. If you don't like
this, use PROC instead. It works similar, but the cache exists only for
the lifetime of PROC.

>> 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.
>>
>>  (add-to-list 'tramp-default-host-alist
>>               `(,tramp-androidsu-method nil "localhost"))
>>
>>
>> I would also add
>>
>>  (add-to-list 'tramp-default-user-alist
>> 	      `(,tramp-androidsu-method nil ,tramp-root-id-string))
>>
>> See also tramp-sh.el.
>>
>> 		     (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"))
>>
>>
>> Pls use the constant for the shell name.
>
> Where's the harm in this, though?  The location of /system/bin/sh will
> never change, so there will never be reason for either users or
> developers to replace it by anything else.

Codong style. I also fail to apply it consequently, but experience shows
that such remote device specific settings shall be customizable. Even if
they *look* like they will never change.

This is from 20 years of Tramp maintainership.

>> 		(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:]]*$"))
>>
>>
>> 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 ...)
>
> tramp-adb-wait-for-output also attempts to detect and delete certain
> character sequences printed by adb, which is inappropriate when the
> shell is being directly run in an Android device.

Yes, this is an ugly hack in tramp-adb.el. I'll rather appreciate
patches for tramp-adb.el which removes this hack forever.

>>                 ;; Disable Unicode.
>>                 (tramp-adb-send-command vec "set +U")
>>
>> Why?
>
> File names with Unicode characters outside the BMP cannot be opened
> otherwise.

Hmm. Then improve the comment, please.

>> 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.
>
> This won't be a problem, since this wrapper only overrides functions
> defined in tramp-adb.el, which I don't anticipate anyone using on
> Android.

I let this open for now. But I still have a bad gut feeling.

>> That's it for the moment.
>
> Okay, thanks.

Best regards, Michael.