[PATCH] home: symlink-manager: Use no-follow version of file-exists?.

  • Done
  • quality assurance status badge
Details
3 participants
  • Andrew Tropin
  • Ludovic Courtès
  • Maxime Devos
Owner
unassigned
Submitted by
Andrew Tropin
Severity
normal
A
A
Andrew Tropin wrote on 7 Apr 2022 10:22
(address . guix-patches@gnu.org)
87zgkxxnvv.fsf@trop.in
* gnu/home/services/symlink-manager.scm (update-symlinks-script): Use
no-follow version of file-exists?.
---
file-exists? returns #f on dangling symlinks, which makes such files
"invisible" during the cleanup process and breaks activation of home
environment.

gnu/home/services/symlink-manager.scm | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)

Toggle diff (47 lines)
diff --git a/gnu/home/services/symlink-manager.scm b/gnu/home/services/symlink-manager.scm
index 6d19258ec7..bb67152e5b 100644
--- a/gnu/home/services/symlink-manager.scm
+++ b/gnu/home/services/symlink-manager.scm
@@ -85,6 +85,13 @@ (define (target-file file)
;; such as "config/fontconfig/fonts.conf" or "bashrc".
(string-append home-directory "/" (preprocess-file file)))
+ (define (no-follow-file-exists? file)
+ "Return #t if file exists, even if it's a dangling symlink."
+ (or (file-exists? file)
+ (and=> (false-if-exception (lstat file))
+ (lambda (x)
+ (equal? (stat:type x) 'symlink)))))
+
(define (symlink-to-store? file)
(catch 'system-error
(lambda ()
@@ -123,7 +130,7 @@ (define (strip file)
(const #t)
(lambda (file stat _) ;leaf
(let ((file (target-file (strip file))))
- (when (file-exists? file)
+ (when (no-follow-file-exists? file)
;; DO NOT remove the file if it is no longer a symlink to
;; the store, it will be backed up later during
;; create-symlinks phase.
@@ -183,7 +190,7 @@ (define (source-file file)
(lambda (file stat result) ;leaf
(let ((source (source-file (strip file)))
(target (target-file (strip file))))
- (when (file-exists? target)
+ (when (no-follow-file-exists? target)
(backup-file (strip file)))
(format #t (G_ "Symlinking ~a -> ~a...")
target source)
@@ -192,7 +199,7 @@ (define (source-file file)
(lambda (directory stat result) ;down
(unless (string=? directory config-file-directory)
(let ((target (target-file (strip directory))))
- (when (and (file-exists? target)
+ (when (and (no-follow-file-exists? target)
(not (file-is-directory? target)))
(backup-file (strip directory)))
--
2.34.0
-----BEGIN PGP SIGNATURE-----

iQJDBAEBCgAtFiEEKEGaxlA4dEDH6S/6IgjSCVjB3rAFAmJOqdQPHGFuZHJld0B0
cm9wLmluAAoJECII0glYwd6wWssP/R3DnmjRX8tROgJDGXixms/N0MahKWhSIAXG
mIxhPmqeYT22RjXDQWxuAK7d0nBWY9CD6yNocQeQ6lBZ5lyiiow+GAqUYqj+vQLO
RWLbuk+1+prUbGDE8OyXO0nVAFv8f7PYVb6P5shL0m4qIfONcwGI70NAW8wx3FaP
1RvzsbZLlNwHRfquB63OV7oi7w7Ku93sJGoIBZpOPD1ezTFi6Es9m60DEDSoNu9y
v6SiQdmMpPa9mljJa6iIT3uYy5x9X3wd1FDvN1nl9ioxFv6l6RQLyg8NQu7cZlOu
AsbA8u3XN9PlTCKRkQPyLAQhhy9oLIA4QILa5z6rI28Y8CX9GcLtn2HrPXGw9y3W
li9YATvZ/27u9f3lpmmviP+1qHLJYE+3byqfiNLWgry7JD1xIs8NWtWJmOdfyk1w
6hJEkr+B1a7rj0AzmQ2yet7BwDUZ9iVTDZnOo66CDeM4EWX5Z6Byx5/83CK/uTok
+dwFzI0/tdTzNUagScGJwPssRz8w3u0U71sG9mGWmyWkjZt/RhKzaEN3dowvBixU
24B+2cnzos/rEE0CWKAJ8X6L/c6YIbkVO5O/mzYxWSluYri7selClDAXbqOCDfS3
+gD6SDsPA6fDzsfb7AtTTPCXxxKS4Z4ODk5FdxwRBteDzEJxBL1NDiyd8hPJwpv3
p0RRM2D5
=cHw6
-----END PGP SIGNATURE-----

M
M
Maxime Devos wrote on 7 Apr 2022 14:28
67cab598e57ecf7cf930d4b2d0568fd2f2f6f95d.camel@telenet.be
Andrew Tropin schreef op do 07-04-2022 om 11:22 [+0300]:
Toggle quote (8 lines)
> +         (define (no-follow-file-exists? file)
> +           "Return #t if file exists, even if it's a dangling
> symlink."
> +           (or (file-exists? file)
> +               (and=> (false-if-exception (lstat file))
> +                      (lambda (x)
> +                        (equal? (stat:type x) 'symlink)))))

Can't this be simplified to

(define (no-follow-file-exists? file)
(false-if-exception (lstat file)))

? Also, do you want to ignore _all_ exceptions, or only the ENOENT and
maybe ENOTDIR system-error?

(catch 'system-error
(lambda () (lstat file) #t)
(lambda e
(if its-a-ENOFILE
#f
(apply throw e))))

More concretely, why is ENOMEM, ENAMETOOLONG and EACCESS ignored here?

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYk7Y2xccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7rUBAQCy6KHGhwNTDiAhMyOjC5I+idJh
JsTtxz00QG6dcX0GzAEAhj9ySML/IWO9whpf5Eh1SIUm70O+OwgBHKXe0BHHhg4=
=qjj6
-----END PGP SIGNATURE-----


A
A
Andrew Tropin wrote on 7 Apr 2022 19:01
87bkxc6d5t.fsf@trop.in
On 2022-04-07 14:28, Maxime Devos wrote:

Toggle quote (15 lines)
> Andrew Tropin schreef op do 07-04-2022 om 11:22 [+0300]:
>> +         (define (no-follow-file-exists? file)
>> +           "Return #t if file exists, even if it's a dangling
>> symlink."
>> +           (or (file-exists? file)
>> +               (and=> (false-if-exception (lstat file))
>> +                      (lambda (x)
>> +                        (equal? (stat:type x) 'symlink)))))
>
> Can't this be simplified to
>
> (define (no-follow-file-exists? file)
> (false-if-exception (lstat file)))
>

Idk how file-exists? works internally, but still expect it to be more
efficient than lstat. That's why I decided to use lstat only as a
"fallback" option in `or` statement.

Toggle quote (12 lines)
> ? Also, do you want to ignore _all_ exceptions, or only the ENOENT and
> maybe ENOTDIR system-error?
>
> (catch 'system-error
> (lambda () (lstat file) #t)
> (lambda e
> (if its-a-ENOFILE
> #f
> (apply throw e))))
>
> More concretely, why is ENOMEM, ENAMETOOLONG and EACCESS ignored here?

You are right, we are interested only in ENOENT here. AFAIK, ENOMEM
shouldn't happen here, but some other can, still we do not handle them
and make function behave the same way as file-exists? do and just return
#f in such cases. However, I'm not sure if file-exists? is a good
example to follow.

Anyway in current implementation handling other codes in
no-follow-file-exists? will not save the day, in case of EACCESS it
doesn't really matter if it will be thrown during the check of existence
or during creation of symlink. But we probably can later rework the
whole symlinking process: check that we have premissions to all files,
we are interested in, and only after that cleanup, backup and symlink.

Toggle quote (4 lines)
>
> Greetings,
> Maxime.

--
Best regards,
Andrew Tropin
-----BEGIN PGP SIGNATURE-----

iQJDBAEBCgAtFiEEKEGaxlA4dEDH6S/6IgjSCVjB3rAFAmJPGN4PHGFuZHJld0B0
cm9wLmluAAoJECII0glYwd6wXBcP/0qJyjIqMZKXH8LHqJoPD3pKz+IU0hClGyih
dpY0/skz2+9JCESryNa4a1sxvYtXyQM0NttL2MWA9PMglQe9XNxcUAcclnkMH3BX
4NKQ/aOMR9vL6KwpIc0h8nPB3CPPyCnSLrSs/DbCbVGQuFP3S61V2p1d7zHpWqK8
i8+c3eYaYBwmzbqSezm57QcZOBZWzo8XISY0MM/K3iZNshf/KE84Gsujl3WJenqV
ELYH4aJDpw+JMtgJhOjO4TTCnB+fxHjlcwwo1Nk8idq3d7iAwj6PyM2zxhTMcsZn
8nWiRXhjpVMWdkRQTuRjxU2af6TPD2tQPKDn/jupai2bl8wWQTRAlUfBDbdzm8M7
Ka6rjXllY9KVvld30IPrkqe1DKOKOPTJKMkfM8Ez5Brp5VbYHP0ubfiO+4FAKslt
MdqHw6mgeFDPwRS5QtqDJgHALKoGKTncbMrSyQm3btWuccXSexqgIR/7C5/pGJz7
GTrg4nmKRprbyNXK1u7heTFrNG9UF0H9jUmXHwdSB8x3cIzYA6QVWXwDekIoMLTs
jdngs3F6RqD/jgKstErE8D/0TjgA+bWNzCzcw3kad37hr86eJN3dkJcsNeb57Ejr
ixCC1u56W6LEHsvAzNf5fweRGVam49nLQCuN+mJ5U4czMPyABUPfGJImihA+ZrFN
gijd1Js6
=kTwY
-----END PGP SIGNATURE-----

M
M
Maxime Devos wrote on 7 Apr 2022 20:17
ab20fec3b3abc3253b3c88463213d2db7dfcee52.camel@telenet.be
Andrew Tropin schreef op do 07-04-2022 om 20:01 [+0300]:
Toggle quote (3 lines)
> You are right, we are interested only in ENOENT here.  AFAIK, ENOMEM
> shouldn't happen here

The kernel should indeed not be out-of-memory. But it can happen!
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYk8qvBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7oWTAP9bvX/Vh+WdDPeXDagZRiGsA1e9
afrH9d7+KSKJMyLLBAEArRhuxpRt601anQCSFGCMpJifAzfTlcTsWAGHDGWIAgI=
=KiUc
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 7 Apr 2022 20:21
be6efed627587bf2a962db2f76615b8de538c2ba.camel@telenet.be
Andrew Tropin schreef op do 07-04-2022 om 20:01 [+0300]:
Toggle quote (4 lines)
> Idk how file-exists? works internally, but still expect it to be more
> efficient than lstat.  That's why I decided to use lstat only as a
> "fallback" option in `or` statement.

Here's the definition, from module/ice-9/boot-9.scm (Guile source
code):

;; For reference, Emacs file-exists-p uses stat in this same way.
(define file-exists?
(if (provided? 'posix)
(lambda (str)
(->bool (stat str #f)))
[non-POSIX code that's not relevant to Guix]))

'file-exists?' just calls 'stat', a variant of 'lstat', so I don't
think there are performance gains to be had here. Well, the
(stat ... #f) might not need to install an exception handler since it
is written in C, but that seems at most a micro-optimisation to me.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYk8rrxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7mNOAP9zSC/W3+lRrOEHQR+3zx3iOIw2
rofk1b43hgEAXNwzMAD9F0dT09a4yVZeHY6YbIRyBAoD8TTqpclCZ9cdindzOQ4=
=Mrny
-----END PGP SIGNATURE-----


A
A
Andrew Tropin wrote on 8 Apr 2022 06:23
878rsg5hkp.fsf@trop.in
On 2022-04-07 20:21, Maxime Devos wrote:

Toggle quote (23 lines)
> Andrew Tropin schreef op do 07-04-2022 om 20:01 [+0300]:
>> Idk how file-exists? works internally, but still expect it to be more
>> efficient than lstat.  That's why I decided to use lstat only as a
>> "fallback" option in `or` statement.
>
> Here's the definition, from module/ice-9/boot-9.scm (Guile source
> code):
>
> ;; For reference, Emacs file-exists-p uses stat in this same way.
> (define file-exists?
> (if (provided? 'posix)
> (lambda (str)
> (->bool (stat str #f)))
> [non-POSIX code that's not relevant to Guix]))
>
> 'file-exists?' just calls 'stat', a variant of 'lstat', so I don't
> think there are performance gains to be had here. Well, the
> (stat ... #f) might not need to install an exception handler since it
> is written in C, but that seems at most a micro-optimisation to me.
>
> Greetings,
> Maxime.

Updated the implementation, which behaves similiar to file-exists?, but
not follow symlinks.
From 92ee52a96d536cba2b3b473f99e8f36646da81fd Mon Sep 17 00:00:00 2001
From: Andrew Tropin <andrew@trop.in>
Date: Thu, 7 Apr 2022 11:22:48 +0300
Subject: [PATCH v2] home: symlink-manager: Use no-follow version of
file-exists?.

* gnu/home/services/symlink-manager.scm (update-symlinks-script): Use
no-follow version of file-exists?.
---
gnu/home/services/symlink-manager.scm | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

Toggle diff (44 lines)
diff --git a/gnu/home/services/symlink-manager.scm b/gnu/home/services/symlink-manager.scm
index 6d19258ec7..e4c931fbee 100644
--- a/gnu/home/services/symlink-manager.scm
+++ b/gnu/home/services/symlink-manager.scm
@@ -85,6 +85,10 @@ (define (target-file file)
;; such as "config/fontconfig/fonts.conf" or "bashrc".
(string-append home-directory "/" (preprocess-file file)))
+ (define (no-follow-file-exists? file)
+ "Return #t if file exists, even if it's a dangling symlink."
+ (->bool (false-if-exception (lstat file))))
+
(define (symlink-to-store? file)
(catch 'system-error
(lambda ()
@@ -123,7 +127,7 @@ (define (strip file)
(const #t)
(lambda (file stat _) ;leaf
(let ((file (target-file (strip file))))
- (when (file-exists? file)
+ (when (no-follow-file-exists? file)
;; DO NOT remove the file if it is no longer a symlink to
;; the store, it will be backed up later during
;; create-symlinks phase.
@@ -183,7 +187,7 @@ (define (source-file file)
(lambda (file stat result) ;leaf
(let ((source (source-file (strip file)))
(target (target-file (strip file))))
- (when (file-exists? target)
+ (when (no-follow-file-exists? target)
(backup-file (strip file)))
(format #t (G_ "Symlinking ~a -> ~a...")
target source)
@@ -192,7 +196,7 @@ (define (source-file file)
(lambda (directory stat result) ;down
(unless (string=? directory config-file-directory)
(let ((target (target-file (strip directory))))
- (when (and (file-exists? target)
+ (when (and (no-follow-file-exists? target)
(not (file-is-directory? target)))
(backup-file (strip directory)))
--
2.34.0
--
Best regards,
Andrew Tropin
-----BEGIN PGP SIGNATURE-----

iQJDBAEBCgAtFiEEKEGaxlA4dEDH6S/6IgjSCVjB3rAFAmJPuMYPHGFuZHJld0B0
cm9wLmluAAoJECII0glYwd6wKD4P/Rc4+9sXARQUG3OSRTnLyLGO8UHO+3RQHxiW
rwBsb6OtWC1oCQjTxufGjU/xeCDWtwxAE+V8UTYB3WMfqPrRXfWjVLR8/3LW27dm
UIFLh34dtqC9rkLjrgOH43fDgLkmxwjuGf2k6hdpDtsv010pu3oSTVFftDejAp/Z
pnkENfcSvcCjuwPcFel+4VSW7+pke9N8VXPFEIMj5JhyxZ0z5d5wtuGV0doW8l4C
k153QB5VouZ4+AFSRQLDGdzqaTpgs7KBnbF0jlkve8Gz/WjMQvr0ErIeUTOzNdEW
Ku6OondiTQQJpnJFm1pqvKB/tztxTKZBEs7rJMM1UPmu7lgrdbYTNlq7OP3xmLb5
UbiKLsjnJaNKZnqqJDarCLhatMthHKQT5txU84cAit3BvpS3PwBjYRoSuMlLjRvb
A3POs3AXd6bqXTycMeeOA8mck7KbeYC8m5i6trvb/76bN0m/I/E46HAF0s1Ojmbh
fkEny4qbhwqa3KSFLSwZeldZnpzIsdlTf3O6aL/o44fIBUK4IDXGI9/7gm8bEpwZ
I/Ws5hH6T3ThbpQ9HO75KNT7z1hD8d6JaB55VZQS2+wMbMzOZT4wR8drWqvds+IP
QOsbWME9lA+imLMsVQ4CS8bJIEBapEK8Qk7F9uM455ssPFehUFgYvQwLvuzHrE/+
pGGSxIW9
=WHvJ
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 9 Apr 2022 23:57
Re: bug#54762: [PATCH] home: symlink-manager: Use no-follow version of file-exists?.
(name . Andrew Tropin)(address . andrew@trop.in)
87lewd7weo.fsf_-_@gnu.org
Hi,

Andrew Tropin <andrew@trop.in> skribis:

Toggle quote (9 lines)
> From 92ee52a96d536cba2b3b473f99e8f36646da81fd Mon Sep 17 00:00:00 2001
> From: Andrew Tropin <andrew@trop.in>
> Date: Thu, 7 Apr 2022 11:22:48 +0300
> Subject: [PATCH v2] home: symlink-manager: Use no-follow version of
> file-exists?.
>
> * gnu/home/services/symlink-manager.scm (update-symlinks-script): Use
> no-follow version of file-exists?.

Applied, thanks!

Ludo’.
Closed
A
A
Andrew Tropin wrote on 11 Apr 2022 07:26
(name . Ludovic Courtès)(address . ludo@gnu.org)
877d7w198f.fsf@trop.in
On 2022-04-09 23:57, Ludovic Courtès wrote:

Toggle quote (17 lines)
> Hi,
>
> Andrew Tropin <andrew@trop.in> skribis:
>
>> From 92ee52a96d536cba2b3b473f99e8f36646da81fd Mon Sep 17 00:00:00 2001
>> From: Andrew Tropin <andrew@trop.in>
>> Date: Thu, 7 Apr 2022 11:22:48 +0300
>> Subject: [PATCH v2] home: symlink-manager: Use no-follow version of
>> file-exists?.
>>
>> * gnu/home/services/symlink-manager.scm (update-symlinks-script): Use
>> no-follow version of file-exists?.
>
> Applied, thanks!
>
> Ludo’.

Maxime and Ludo, thank you!

--
Best regards,
Andrew Tropin
-----BEGIN PGP SIGNATURE-----

iQJDBAEBCgAtFiEEKEGaxlA4dEDH6S/6IgjSCVjB3rAFAmJTvAEPHGFuZHJld0B0
cm9wLmluAAoJECII0glYwd6wvggP/33uzwkjUIpkmuDcP18PWnwmYn+0pflV+Y4n
/PCKg6EEl9aNURdrGoe5kCrK12CMLMIQUxvzAjRBgRuO0FsEsfKEL/SAwtqk+uoj
CzcIME0sYpV0Sj53IzXg1hixeo9PjnWOXJ1IDt1Chfu5PcE2V5QBteWNiUXw/bUE
ZblS5EiVqvRzZLMMZjtkqcDeecVqlxjekSW8GkjSl+0Drtg/3gdcXGk+NiBngbu4
0eXH9f2tcHkErG5o1lkOc1RWfQWnQ6wnvBZWjkCuO0R137YgvLAD46X2NydRMnRT
UNtjVNCBfunLthzOqPIhLIRoe14+xwXO9ZUZJuCt7yDmwcv8c64Xh9UEaMNQr1JV
rbdQwChjMPQgmu0zuUqbFWdFJ2vAZ/IG4gTLtOEf8jdt436bpy5SvfwnNPhFEBcv
+V2vH1/EwPCXhYCiPpRfu8HJLCGSi1iqQmV+k4d5f7vsLa42Xu16sozKpOqd+7dd
+QVbTKUJZESLFiIdRrYyZnh1bfKHb9jRtRSkME6SQElJIQiB6mD8REpf+x5p4Gvx
bTNWj9sKHF8GRGq80JPQE5LoJHsQf6d57C+NHKbyn4oxzoUMlx/ObI+xgEZ58DLR
TTkuEdfcWRiog53p63vrcWq8x3sPLy2iHDA7zsDdX6DdKuSJG6EpK1rNCdou/tJc
mLlVuKFp
=IOhA
-----END PGP SIGNATURE-----

Closed
?