From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Po Lu Newsgroups: gmane.emacs.devel Subject: Re: master babe6a5e948 1/2: Introduce a new TRAMP method `androidsu' Date: Sun, 03 Mar 2024 11:10:15 +0800 Message-ID: <87sf182ep4.fsf@yahoo.com> References: <170892805989.23297.10872025500935567738@vcs2.savannah.gnu.org> <20240226061420.EF1D4C00231@vcs2.savannah.gnu.org> <875xybtxq0.fsf@gmx.de> <87a5nn61k7.fsf@yahoo.com> <87r0gzpopy.fsf@gmx.de> <875xyb5wct.fsf@yahoo.com> <87edcsptvh.fsf@gmx.de> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="5294"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: emacs-devel@gnu.org To: Michael Albinus Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Mar 03 04:11:45 2024 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rgcGP-0001F6-BV for ged-emacs-devel@m.gmane-mx.org; Sun, 03 Mar 2024 04:11:45 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rgcFa-0000pD-Ge; Sat, 02 Mar 2024 22:10:54 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rgcFZ-0000p0-OU for emacs-devel@gnu.org; Sat, 02 Mar 2024 22:10:53 -0500 Original-Received: from sonic317-33.consmr.mail.ne1.yahoo.com ([66.163.184.44]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rgcFW-00025O-S2 for emacs-devel@gnu.org; Sat, 02 Mar 2024 22:10:53 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1709435446; bh=+bJEzyoXKOwx+hNdEECmTlM/J5WJkooj33IiMFAj1vw=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From:Subject:Reply-To; b=YYIKqpHgt048M4qg8uqzr7XiX7rJ7Jp5YuHvHQpwvePzPTTb/2FjCiOcOEOf9gtbaxjgzfxIBYWta9yTmZKyPbYLYjj/txnbge8mVgcjGhiRppAG7/L7KNFbs8qgPKLU1P7Zxa5YpKjlA3W0OHf3b0uIMCi59Fe4An7L2aRdbacV1U+x+EfqSFj9vXmuBHzAiFGGNnFKbmm3Pi+5wpDs7Hk3hbYgN1CaYiDDl1xAFWDmS/bxF8m28HH6WIMFOhYEQcVwRe9qlVnQ7ili9OUlgJalBkyoOgcYGVT3VWyFlk1jpA7BDYxW8y9y4Jpy+83xN8mhvygdfAzMmc67XkOMKA== X-SONIC-DKIM-SIGN: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1709435446; bh=Gi3KgxngOgucyj+vIQZbC8KaxaZ7oRpoOYzCwkOPs9j=; h=X-Sonic-MF:From:To:Subject:Date:From:Subject; b=XRQheghRGy6ueGWZBcKnSM4teL28dpySm0BjIS1veN7wo+Dky6cv6AMICWxMabBAqBzn1qlr6N/FMhPTumHBubZu01tEOB0aBf0hAaHHaw99wYOCY+t3AERDzU6di3mwInBI/mXHNmUuoYybkePn6DJS6Gp7pqd5fkuXcT1rm4t3oC1M9GnWPDg3K2G/lfMtmgLQyhF1FGimUKgXNIpY/EcmpAA/1XBkKUlboM1mIfaZpaIAkCdKjhtARPA4RfcF7+Q0+qE20Db/FQQ0OJfulI2CzH86FSi4sMiRTOc0+V4VJn8E/lVszY7hxt+1hychwdAA09YhRnyVtgAfpsxLEw== X-YMail-OSG: brpyfuYVM1mqJBV9cc1q8wz.rtUmK3mIDXp0qNeMomdYnPZBiT4kB0edbsSnwhc uV4haQoQQXJNYJ9TWySQTSA5FMjsxNxWSnYOJOlNHvXMmBKE4JCYr_22x7Mbc_6Fi8VyrY1yVvK2 n5ILRvUhKttgBshan844X7rxcQL_mPqkJh2X4WriX7FZh2qpEfedom5ACUcYF2hnbU_YtC6j0xE_ CN7V_G7Q_6C76eeyAxsA_e_W6fI.ciSOMyqmJlcADzb8XOayRxwkQNeSptbAgvRZYaRKfi3N_5lq yxYozU6PFDIlrLqXHn3ERae6qtICZVR5_DgKAM.YCbHOWAgj4Ywa1WzsLm3rgZRWHcTALvgb5cHm _YU9Vl6HcPdAEstWkKr2DsnQ_DvTKXmz36TMpnVGde0rkEmpeQF7u8Tiv5JdXbEsru89nL64EKtk U7xfHfDnAC4faiGYLLIxQQ5XSaXgEtuhe2gha9sdObqLr5uRIY8Y9.sZefmBXbueqPgF0A7KpNIm w0dhKjPEJooqZZOXuwo0z7bfbiFS_oRWbqddQ.gbqXxOYYLgb6Q0bPFAigyf7TgZpmkBY77W4cqr uV6JjsGisewWmMlyPTraNOGjmrw4Yfl5P0EFReGAf.gY6fFCk_55gwEMPIRMjNToVxuQ2suid7p_ pvdn97tjA0XAewIh8XnozWRo6gApBbzLJcRB3F8r.nsj12QREgd.ELZZT0Yu7Rwvj5G5kc_H.CWi _BhRV.0zhEUZFeUa9QPnkQwNe21Eq_qvadaNEYKaD07Fe5fHJ9EVA6a7_HJKtT1px21Ywp7shlE4 G0FKPxFFtfUbo7Udq..sZLbSrykgHdLVWJWqLT9tuX X-Sonic-MF: X-Sonic-ID: a7112a88-712b-45b1-9aa4-dff1707dfc1c Original-Received: from sonic.gate.mail.ne1.yahoo.com by sonic317.consmr.mail.ne1.yahoo.com with HTTP; Sun, 3 Mar 2024 03:10:46 +0000 Original-Received: by hermes--production-sg3-6dc75bc8fb-n9pfz (Yahoo Inc. Hermes SMTP Server) with ESMTPA ID c967697925a1195b6bbff6cf0596de89; Sun, 03 Mar 2024 03:10:40 +0000 (UTC) In-Reply-To: <87edcsptvh.fsf@gmx.de> (Michael Albinus's message of "Sat, 02 Mar 2024 15:53:54 +0100") X-Mailer: WebService/1.1.22103 mail.backend.jedi.jws.acl:role.jedi.acl.token.atz.jws.hermes.yahoo Received-SPF: pass client-ip=66.163.184.44; envelope-from=luangruo@yahoo.com; helo=sonic317-33.consmr.mail.ne1.yahoo.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:316718 Archived-At: Michael Albinus 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-conne= ction))))) > > > 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 =E2=80=98su=E2=80=99, =E2=80=98sudo=E2=80= =99, =E2=80=98doas=E2=80=99 and =E2=80=98sg=E2=80=99" or "3.10 > Using Android" section, and not in "3.4 Combining =E2=80=98ssh=E2=80=99 o= r =E2=80=98plink=E2=80=99 with > =E2=80=98su=E2=80=99, =E2=80=98sudo=E2=80=99 or =E2=80=98doas=E2=80=99". = 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.