diff mbox series

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

Message ID 87zgkxxnvv.fsf@trop.in
State Accepted
Headers show
Series [bug#54762] home: symlink-manager: Use no-follow version of file-exists?. | expand

Commit Message

Andrew Tropin April 7, 2022, 8:22 a.m. UTC 
* 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(-)

Comments

M April 7, 2022, 12:28 p.m. UTC | #1 
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)))

?  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.
Andrew Tropin April 7, 2022, 5:01 p.m. UTC | #2 
On 2022-04-07 14:28, Maxime Devos wrote:

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

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

>
> Greetings,
> Maxime.
M April 7, 2022, 6:17 p.m. UTC | #3 
Andrew Tropin schreef op do 07-04-2022 om 20:01 [+0300]:
> 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!
M April 7, 2022, 6:21 p.m. UTC | #4 
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.
diff mbox series

Patch

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