[PATCH core-updates] Move 'mkdir-p/perms' to gnu/build/utils.scm

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Maxim Cournoyer
  • Maxime Devos
Owner
unassigned
Submitted by
Maxime Devos
Severity
normal
M
M
Maxime Devos wrote on 19 Jan 2021 19:42
(address . guix-patches@gnu.org)
8dda4413505b28fedb9588a4064812fe69c19a37.camel@telenet.be
Hi Guix,

This is the patch I talked about on IRC. It moves the various inline
definitions of 'mkdir-p/perms' from gnu/services/... to gnu/build/utils.scm.
I've also written a few tests. As this change entails a world rebuild,
this should be applied to core-updates instead of master (as civodul
pointed out).

`make check TESTS=tests/build-utils.scm` succeeds. Building a few packages
for testing will take some time though (due to the world rebuild).

Plenty of parentheses,
Maxime
--
Maxime Devos <maximedevos@telenet.be>
PGP Key: C1F3 3EE2 0C52 8FDB 7DD7 011F 49E3 EE22 1917 25EE
Freenode handle: mdevos
From 7611565fcee641f83dd2eadbe7f573c0b2fe4240 Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Tue, 19 Jan 2021 18:58:48 +0100
Subject: [PATCH 1/2] utils: Add 'mkdir-p/perms'

* guix/build/utils.scm (mkdir-p/perms): New procedure.
* tests/build-utils.scm: Add test for 'mkdir-p/perms'.
---
guix/build/utils.scm | 10 +++++++++
tests/build-utils.scm | 47 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+)

Toggle diff (93 lines)
diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 419c10195b..9f7b89d9bc 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -59,6 +59,7 @@
reset-gzip-timestamp
with-directory-excursion
mkdir-p
+ mkdir-p/perms
install-file
make-file-writable
copy-recursively
@@ -307,6 +308,15 @@ preserve FILE's modification time."
(apply throw args))))))
(() #t))))
+(define (mkdir-p/perms directory owner perms)
+ "Create directory DIR and all its ancestors.
+Also set its user and group to OWNER, and its
+permission bits to PERMS. OWNER must be an
+password database entry as returned by getpwent."
+ (mkdir-p directory)
+ (chown directory (passwd:uid owner) (passwd:gid owner))
+ (chmod directory perms))
+
(define (install-file file directory)
"Create DIRECTORY if it does not exist and copy FILE in there under the same
name."
diff --git a/tests/build-utils.scm b/tests/build-utils.scm
index 654b480ed9..557751c858 100644
--- a/tests/build-utils.scm
+++ b/tests/build-utils.scm
@@ -1,6 +1,7 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2012, 2015, 2016, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2019 Ricardo Wurmus <rekado@elephly.net>
+;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -241,4 +242,50 @@ print('hello world')"))
"/some/other/path")))
#f)))))
+;; this also tests mkdir-p itself
+(let ((owner (getpwuid (getuid)))
+ (test-bits '(#o700 #o070 #o007)))
+ (test-assert "mkdir-p/perms, creates directory"
+ (call-with-temporary-directory
+ (lambda (directory)
+ (let ((foo (string-append directory "/a/dir")))
+ (mkdir-p/perms foo owner #o700)
+ (file-exists? foo)))))
+ ;; Unfortunately, testing owner != user requires root,
+ ;; and thus cannot be tested here on Linux systems.
+ ;; TODO: test this on GNU/Hurd.
+ (test-equal "mkdir-p/perms, set permission bits of new directories"
+ test-bits
+ (map (lambda (bits)
+ (call-with-temporary-directory
+ (lambda (directory)
+ (let ((foo (string-append directory "/a/dir")))
+ (mkdir-p/perms foo owner bits)
+ ;; Prevent ‘warning: failed to delete /tmp/.../dir: Permission denied’
+ ;; noise in the logs.
+ (let ((perms (stat:perms (stat foo))))
+ (chmod foo #o700)
+ perms)))))
+ test-bits))
+ (test-equal "mkdir-p/perms, reset permission bits of old directories"
+ test-bits
+ (map (lambda (bits)
+ (call-with-temporary-directory
+ (lambda (directory)
+ (let ((foo (string-append directory "/a/dir")))
+ (mkdir-p/perms foo owner #o000)
+ (mkdir-p/perms foo owner bits)
+ (let ((perms (stat:perms (stat foo))))
+ (chmod foo #o700)
+ perms)))))
+ test-bits))
+ (test-equal "mkdir-p, use umask for creating parent directories"
+ (logxor #o777 (umask))
+ (call-with-temporary-directory
+ (lambda (directory)
+ (let* ((foo (string-append directory "/a/dir"))
+ (foo-parent (dirname foo)))
+ (mkdir-p/perms foo owner #o777)
+ (stat:perms (stat foo-parent)))))))
+
(test-end)
--
2.30.0
From 6ebc5f9e390af1d2efaee1c6640724b358434029 Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Tue, 19 Jan 2021 19:19:54 +0100
Subject: [PATCH 2/2] gnu: remove inline 'mkdir-p/perms' definitions

* gnu/services/mail.scm (%dovecot-activation): Leave this
anomalous definition for someone else to figure out.
* gnu/services/dns.scm (%knot-activation): Remove
inline definition of 'mkdir-p/perms'.
* gnu/services/cups.scm (%cups-activation): Likewise.
---
gnu/services/cups.scm | 4 ----
gnu/services/dns.scm | 4 ----
gnu/services/mail.scm | 13 ++++++++-----
3 files changed, 8 insertions(+), 13 deletions(-)

Toggle diff (65 lines)
diff --git a/gnu/services/cups.scm b/gnu/services/cups.scm
index 17ed04e58b..5099bbe421 100644
--- a/gnu/services/cups.scm
+++ b/gnu/services/cups.scm
@@ -874,10 +874,6 @@ IPP specifications.")
(with-imported-modules '((guix build utils))
#~(begin
(use-modules (guix build utils))
- (define (mkdir-p/perms directory owner perms)
- (mkdir-p directory)
- (chown directory (passwd:uid owner) (passwd:gid owner))
- (chmod directory perms))
(define (build-subject parameters)
(string-concatenate
(map (lambda (pair)
diff --git a/gnu/services/dns.scm b/gnu/services/dns.scm
index b339eb0619..cf8e9dac7f 100644
--- a/gnu/services/dns.scm
+++ b/gnu/services/dns.scm
@@ -609,10 +609,6 @@
(define (knot-activation config)
#~(begin
(use-modules (guix build utils))
- (define (mkdir-p/perms directory owner perms)
- (mkdir-p directory)
- (chown directory (passwd:uid owner) (passwd:gid owner))
- (chmod directory perms))
(mkdir-p/perms #$(knot-configuration-run-directory config)
(getpwnam "knot") #o755)
(mkdir-p/perms "/var/lib/knot" (getpwnam "knot") #o755)
diff --git a/gnu/services/mail.scm b/gnu/services/mail.scm
index c0f6371104..e17be3197c 100644
--- a/gnu/services/mail.scm
+++ b/gnu/services/mail.scm
@@ -1484,7 +1484,10 @@ greyed out, instead of only later giving \"not selectable\" popup error.
dovecot-configuration-fields)))))))
#~(begin
(use-modules (guix build utils))
- (define (mkdir-p/perms directory owner perms)
+ ;; XXX someone please take a look
+ ;; if the hardcoding of /var/run/dovecot
+ ;; is intended, or a bug. idk
+ (define (mkdir-p/perms-xxx directory owner perms)
(mkdir-p directory)
(chown "/var/run/dovecot" (passwd:uid owner) (passwd:gid owner))
(chmod directory perms))
@@ -1529,12 +1532,12 @@ greyed out, instead of only later giving \"not selectable\" popup error.
(format (current-error-port)
"Failed to create public key at ~a.\n" public-key)))))
(let ((user (getpwnam "dovecot")))
- (mkdir-p/perms "/var/run/dovecot" user #o755)
- (mkdir-p/perms "/var/lib/dovecot" user #o755)
- (mkdir-p/perms "/etc/dovecot" user #o755)
+ (mkdir-p/perms-xxx "/var/run/dovecot" user #o755)
+ (mkdir-p/perms-xxx "/var/lib/dovecot" user #o755)
+ (mkdir-p/perms-xxx "/etc/dovecot" user #o755)
(copy-file #$(plain-file "dovecot.conf" config-str)
"/etc/dovecot/dovecot.conf")
- (mkdir-p/perms "/etc/dovecot/private" user #o700)
+ (mkdir-p/perms-xxx "/etc/dovecot/private" user #o700)
(create-self-signed-certificate-if-absent
#:private-key "/etc/dovecot/private/default.pem"
#:public-key "/etc/dovecot/default.pem"
--
2.30.0
-----BEGIN PGP SIGNATURE-----

iI0EABYIADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYAcoBBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7p9bAQCRkEY8GR9Zk1gNIsukVcvFe8lD
9sgIkr1exauJa5tOwAEApZNzOIOncHDBjRv9qk6iv5M5gISUGcY3+pvYRmHYuwc=
=Ru3B
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 19 Jan 2021 19:52
(address . 45991@debbugs.gnu.org)
1fb485e7e14e452c96673bebe60d8b14bb536e80.camel@telenet.be
I forgot to mention some atomicity issues the
current mkdir-p/perms has. Excerpt from IRC:

(15:17:25) mdevos: I see ‘mkdir-p/perms’ doesn't create the directory
and set the permissions atomically; there's a tiny window where a
freshly-created directory has the permissions that would be expected
from the umask. Is this something to be concerned about (and to be
fixed in the patch)?
(15:40:46) civodul: mdevos: it's a good idea to be concerned about
this, yes :-)
(15:41:27) civodul: in general, given that changes in (guix build utils)
take time to trickle in, we should be extra cautious about interfaces and
implementation details

This patch doesn't address these potential issues.
Also, %dovecot-activation has an anomalous mkdir-p/perms.
-----BEGIN PGP SIGNATURE-----

iI0EABYIADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYAcqWBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7vnSAQCqYVBF6vm/ZeewRD8UKbL8jKGn
zIhwKOaYD06kpp0usgEApjg0zx9C+lV55cc2WMUnOJHymEKxXc8ZlKU2KzfFtgs=
=X7tY
-----END PGP SIGNATURE-----


L
L
Ludovic Courtès wrote on 22 Apr 2021 10:56
Re: bug#45991: [PATCH core-updates] Move 'mkdir-p/perms' to gnu/build/utils.scm
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 45991@debbugs.gnu.org)
87mttq1yv6.fsf@gnu.org
Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

Toggle quote (6 lines)
> This is the patch I talked about on IRC. It moves the various inline
> definitions of 'mkdir-p/perms' from gnu/services/... to gnu/build/utils.scm.
> I've also written a few tests. As this change entails a world rebuild,
> this should be applied to core-updates instead of master (as civodul
> pointed out).

Since (gnu build activation) now has a variant of ‘mkdir-p/perms’ that
verifies that directory components are not symlinks, should we still
include this one in (guix build utils)?

Thanks for all the parens! :-)

Ludo’.
M
M
Maxim Cournoyer wrote on 20 Oct 2023 04:38
control message for bug #45991
(address . control@debbugs.gnu.org)
87lebym31h.fsf@gmail.com
tags 45991 wontfix
close 45991
quit
M
M
Maxim Cournoyer wrote on 20 Oct 2023 04:38
Re: bug#45991: [PATCH core-updates] Move 'mkdir-p/perms' to gnu/build/utils.scm
(name . Ludovic Courtès)(address . ludo@gnu.org)
87jzrim30y.fsf_-_@gmail.com
Hi,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (16 lines)
> Hi Maxime,
>
> Maxime Devos <maximedevos@telenet.be> skribis:
>
>> This is the patch I talked about on IRC. It moves the various inline
>> definitions of 'mkdir-p/perms' from gnu/services/... to gnu/build/utils.scm.
>> I've also written a few tests. As this change entails a world rebuild,
>> this should be applied to core-updates instead of master (as civodul
>> pointed out).
>
> Since (gnu build activation) now has a variant of ‘mkdir-p/perms’ that
> verifies that directory components are not symlinks, should we still
> include this one in (guix build utils)?
>
> Thanks for all the parens! :-)

I guess (gnu build activation) is an appropriate home, given configuring
permissions is not typically useful for packaging purposes.

--
Thanks,
Maxim
Closed
?