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

Done

Details

3 participants

Andrew Tropin
Ludovic Courtès
Maxime Devos

Owner: unassigned

Submitted by: Andrew Tropin

Severity: normal

Debbugs page

Andrew Tropin wrote 3 years ago

Recipients:(address . guix-patches@gnu.org)

Message-ID: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

Maxime Devos wrote 3 years ago

Recipients:

Message-ID: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

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

Andrew Tropin wrote 3 years ago

Recipients:

Message-ID: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

Maxime Devos wrote 3 years ago

Recipients:

Message-ID: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-----

Maxime Devos wrote 3 years ago

Recipients:

Message-ID: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-----

Andrew Tropin wrote 3 years ago

Recipients:

Message-ID: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

Ludovic Courtès wrote 3 years ago

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

Recipients:(name . Andrew Tropin)(address . andrew@trop.in)

Message-ID: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

Andrew Tropin wrote 3 years ago

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID: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

Closed

Your comment

This issue is archived.

To comment on this conversation send an email to 54762@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it

mumi current 54762

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags

mumi command -t +confirmed -t +easy

Or, remove the moreinfo tag and set the help tag

mumi command -t -moreinfo -t +help

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date