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.