[-- Attachment #1.1: Type: text/plain, Size: 646 bytes --] When an operating-system contains multiple users or groups with the same name, instantiating it with `guix system` does not cause a validation failure, nor are the duplicate entries filtered from the resulting /etc files. This duplication can happen in a few different ways: - both entries are manually included in the "users" or "groups" fields of the operating-system - a manually-specified entry collides with an entry defined by a service (via an account-service-type extension) - multiple services define entries that collide with each other Steps to reproduce: call "guix system container" with the attached operating-system definition. [-- Attachment #1.2: Type: text/html, Size: 805 bytes --] [-- Attachment #2: duplicate-users-and-groups.scm --] [-- Type: application/octet-stream, Size: 1827 bytes --] (use-modules (gnu)) (use-service-modules networking ssh) (use-package-modules screen ssh) (operating-system (host-name "komputilo") (timezone "Europe/Berlin") (locale "en_US.utf8") (bootloader (bootloader-configuration (bootloader grub-bootloader) (target "/dev/sdX"))) (file-systems (cons (file-system (device (file-system-label "my-root")) (mount-point "/") (type "ext4")) %base-file-systems)) (users (append (list ;; Two manually-specified users with the same name (user-account (name "alice") (comment "Alice 1") (group "users")) (user-account (name "alice") (comment "Alice 2") (group "users")) ;; A manually-specified user with the same name as a ;; user defined by the openssh-service. (user-account (name "sshd") (comment "Secure shell user") (group "sshd") (system? #t))) %base-user-accounts)) (groups (append (list ;; Two manually-specified groups with the same name (user-group (name "power-users")) (user-group (name "power-users")) ;; A manually-specified group with the same name as ;; a group defined by the openssh-service. (user-group (name "sshd"))) %base-groups)) (services (append (list (service dhcp-client-service-type) (service openssh-service-type (openssh-configuration (openssh openssh-sans-x) (port-number 2222)))) %base-services)))
*gnu/system/shadow.scm (assert-unique-account-names) (assert-unique-group-names): New variables. (account-activation): Use them here. --- gnu/system/shadow.scm | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm index a69339bc07..61562f225e 100644 --- a/gnu/system/shadow.scm +++ b/gnu/system/shadow.scm @@ -222,6 +222,32 @@ for a colorful Guile experience.\\n\\n\"))))\n")) (rename-file ".nanorc" ".config/nano/nanorc")) #t)))) +(define (assert-unique-account-names users) + (let loop ((names '()) + (users users)) + (unless (null? users) + (let ((name (user-account-name (car users)))) + (if (member name names) + (raise (condition + (&message + (message + (format #f (G_ "account with name '~a' found twice") + name))))) + (loop (cons name names) (cdr users))))))) + +(define (assert-unique-group-names groups) + (let loop ((names '()) + (groups groups)) + (unless (null? groups) + (let ((name (user-account-name (car groups)))) + (if (member name names) + (raise (condition + (&message + (message + (format #f (G_ "group with name '~a' found twice") + name))))) + (loop (cons name names) (cdr groups))))))) + (define (assert-valid-users/groups users groups) "Raise an error if USERS refer to groups not listed in GROUPS." (let ((groups (list->set (map user-group-name groups)))) @@ -292,6 +318,8 @@ group." (define group-specs (map user-group->gexp groups)) + (assert-unique-account-names accounts) + (assert-unique-group-names groups) (assert-valid-users/groups accounts groups) ;; Add users and user groups. -- 2.29.2
[-- Attachment #1: Type: text/plain, Size: 822 bytes --] Hi Leo, I agree that this is a good idea. Please use (ice-9 match) instead of car and cdr. Something among these lines would be more transparent: (define (find-duplicates list accessor) (match list (() '()) ((head . tail) (if (member head tail accessor) ; (srfi srfi-1) member (cons head (find-duplicates tail accessor)) (find-duplicates tail accessor))))) (find-duplicates users (lambda (a b) (string=? (user-account-name a) (user-account-name b))) (I think one could also use srfi-1 delete-duplicates and then compare the lengths. Then the entire thing is a one-liner--the only complication is to find the duplicates again after doing it (for the error message)) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --]
*gnu/system/shadow.scm (find-duplicates): New variable. (assert-unique-account-names, assert-unique-group-names): New variables. (account-activation): Use them here. --- gnu/system/shadow.scm | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm index a69339bc07..3a5ea4dc70 100644 --- a/gnu/system/shadow.scm +++ b/gnu/system/shadow.scm @@ -34,6 +34,7 @@ #:use-module ((gnu packages admin) #:select (shadow)) #:use-module (gnu packages bash) + #:use-module (ice-9 match) #:use-module (srfi srfi-1) #:use-module (srfi srfi-26) #:use-module (srfi srfi-34) @@ -222,6 +223,38 @@ for a colorful Guile experience.\\n\\n\"))))\n")) (rename-file ".nanorc" ".config/nano/nanorc")) #t)))) +(define (find-duplicates list =) + (match list + ('() '()) + ((first . rest) + (if (member first rest =) ; (srfi srfi-1) member + (cons first (find-duplicates rest =)) + (find-duplicates rest =))))) + +(define (assert-unique-account-names users) + (for-each + (lambda (account) + (raise (condition + (&message + (message + (format #f (G_ "account with name '~a' found twice.") + (user-account-name account))))))) + (find-duplicates users (lambda (alice bob) + (string=? (user-account-name alice) + (user-account-name bob)))))) + +(define (assert-unique-group-names groups) + (for-each + (lambda (group) + (raise (condition + (&message + (message + (format #f (G_ "group with name '~a' found twice.") + (user-group-name group))))))) + (find-duplicates groups (lambda (red blue) + (string=? (user-group-name red) + (user-group-name blue)))))) + (define (assert-valid-users/groups users groups) "Raise an error if USERS refer to groups not listed in GROUPS." (let ((groups (list->set (map user-group-name groups)))) @@ -292,6 +325,8 @@ group." (define group-specs (map user-group->gexp groups)) + (assert-unique-account-names accounts) + (assert-unique-group-names groups) (assert-valid-users/groups accounts groups) ;; Add users and user groups. -- 2.29.2
Hi, Leo Prikler <leo.prikler@student.tugraz.at> skribis: > *gnu/system/shadow.scm (find-duplicates): New variable. > (assert-unique-account-names, assert-unique-group-names): New variables. > (account-activation): Use them here. [...] > +(define (find-duplicates list =) > + (match list > + ('() '()) This should be: (match list (() '()) …) I’m surprised '() works as a pattern. > + ((first . rest) > + (if (member first rest =) ; (srfi srfi-1) member > + (cons first (find-duplicates rest =)) > + (find-duplicates rest =))))) Note that this is quadratic; it’s fine as long as we don’t have “too many” users, which may be the case in general. > +(define (assert-unique-account-names users) > + (for-each > + (lambda (account) > + (raise (condition > + (&message > + (message > + (format #f (G_ "account with name '~a' found twice.") > + (user-account-name account))))))) > + (find-duplicates users (lambda (alice bob) > + (string=? (user-account-name alice) > + (user-account-name bob)))))) ‘for-each’ looks awkward since we’ll stop on the first one. How about something like: (define (assert-unique-account-names users) (match (find-duplicates things …) (() #t) (lst (raise (formatted-message (G_ "the following accounts appear more than once:~{ ~a~}~%" lst)))))) ? Thanks! Ludo’.
Hi, Am Mittwoch, den 06.01.2021, 10:56 +0100 schrieb Ludovic Courtès: > Hi, > > Leo Prikler <leo.prikler@student.tugraz.at> skribis: > > > *gnu/system/shadow.scm (find-duplicates): New variable. > > (assert-unique-account-names, assert-unique-group-names): New > > variables. > > (account-activation): Use them here. > > [...] > > > +(define (find-duplicates list =) > > + (match list > > + ('() '()) > > This should be: > > (match list > (() '()) > …) > > I’m surprised '() works as a pattern. I think it's because matching literals works, but you're right. > > + ((first . rest) > > + (if (member first rest =) ; (srfi srfi-1) member > > + (cons first (find-duplicates rest =)) > > + (find-duplicates rest =))))) > > Note that this is quadratic; it’s fine as long as we don’t have “too > many” users, which may be the case in general. It is indeed quadratic, but would there even be an n log n solution? I've once done an n log n sort+delete-duplicates!, perhaps that'd be a nicer solution here? > > +(define (assert-unique-account-names users) > > + (for-each > > + (lambda (account) > > + (raise (condition > > + (&message > > + (message > > + (format #f (G_ "account with name '~a' found > > twice.") > > + (user-account-name account))))))) > > + (find-duplicates users (lambda (alice bob) > > + (string=? (user-account-name alice) > > + (user-account-name bob)))))) > > ‘for-each’ looks awkward since we’ll stop on the first one. How > about > something like: > > (define (assert-unique-account-names users) > (match (find-duplicates things …) > (() #t) > (lst > (raise (formatted-message (G_ "the following accounts appear > more than once:~{ ~a~}~%" > lst)))))) > > ? That'd be weird for duplicate duplicates, hence just reporting the first. Of course we could always count occurrences by allocating a local hash table and then do some fancy hash-map->list conversion. If we do use hash-tables, perhaps this could even be a linear algorithm? Regards, Leo
Hi, Leo Prikler <leo.prikler@student.tugraz.at> skribis: >> > + ((first . rest) >> > + (if (member first rest =) ; (srfi srfi-1) member >> > + (cons first (find-duplicates rest =)) >> > + (find-duplicates rest =))))) >> >> Note that this is quadratic; it’s fine as long as we don’t have “too >> many” users, which may be the case in general. > It is indeed quadratic, but would there even be an n log n solution? > I've once done an n log n sort+delete-duplicates!, perhaps that'd be a > nicer solution here? You could first build a hash table or vhash or set with all the names, then traverse again the list of names and check whether they’re in that table. That’d be linear (assuming the table is well balanced), but the constant factor would be higher. >> (define (assert-unique-account-names users) >> (match (find-duplicates things …) >> (() #t) >> (lst >> (raise (formatted-message (G_ "the following accounts appear >> more than once:~{ ~a~}~%" >> lst)))))) >> >> ? > That'd be weird for duplicate duplicates, hence just reporting the > first. You could do (delete-duplicates lst) in the message above? Thanks, Ludo’.
Hi, Am Mittwoch, den 06.01.2021, 14:32 +0100 schrieb Ludovic Courtès: > Hi, > > Leo Prikler <leo.prikler@student.tugraz.at> skribis: > > > > > + ((first . rest) > > > > + (if (member first rest =) ; (srfi srfi-1) member > > > > + (cons first (find-duplicates rest =)) > > > > + (find-duplicates rest =))))) > > > > > > Note that this is quadratic; it’s fine as long as we don’t have > > > “too > > > many” users, which may be the case in general. > > It is indeed quadratic, but would there even be an n log n > > solution? > > I've once done an n log n sort+delete-duplicates!, perhaps that'd > > be a > > nicer solution here? > > You could first build a hash table or vhash or set with all the > names, > then traverse again the list of names and check whether they’re in > that > table. That’d be linear (assuming the table is well balanced), but > the > constant factor would be higher. Yeah, I think the hash table solution would make the most sense here. Since VHashes are based on VLists, they're not actually purely functional, are they? > > > (define (assert-unique-account-names users) > > > (match (find-duplicates things …) > > > (() #t) > > > (lst > > > (raise (formatted-message (G_ "the following accounts > > > appear > > > more than once:~{ ~a~}~%" > > > lst)))))) > > > > > > ? > > That'd be weird for duplicate duplicates, hence just reporting the > > first. > > You could do (delete-duplicates lst) in the message above? Sure, but that'd be O(n^2) on top of O(n^2), which is less than ideal. I think I'll try working on a hash-based implementation for now. Regards, Leo
*gnu/system/shadow.scm (find-duplicates): New variable. (assert-unique-account-names, assert-unique-group-names): New variables. (account-activation): Use them here. --- gnu/system/shadow.scm | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm index a69339bc07..4dbd578e1e 100644 --- a/gnu/system/shadow.scm +++ b/gnu/system/shadow.scm @@ -20,6 +20,7 @@ ;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>. (define-module (gnu system shadow) + #:use-module ((guix diagnostics) #:select (formatted-message)) #:use-module (guix records) #:use-module (guix gexp) #:use-module (guix store) @@ -34,6 +35,7 @@ #:use-module ((gnu packages admin) #:select (shadow)) #:use-module (gnu packages bash) + #:use-module (ice-9 match) #:use-module (srfi srfi-1) #:use-module (srfi srfi-26) #:use-module (srfi srfi-34) @@ -222,6 +224,40 @@ for a colorful Guile experience.\\n\\n\"))))\n")) (rename-file ".nanorc" ".config/nano/nanorc")) #t)))) +(define (find-duplicates list) + (let loop ((table (make-hash-table)) + (list list)) + (match list + (() + (hash-fold (lambda (key value seed) + (if (> value 1) + (cons key seed) + seed)) + '() + table)) + ((first . rest) + (hash-set! table first + (1+ (hash-ref table first 0))) + (loop table rest))))) + +(define (assert-unique-account-names users) + (match (find-duplicates (map user-account-name users)) + (() *unspecified*) + (duplicates + (raise + (formatted-message + (G_ "the following accounts appear more than once:~{ ~a~}~%") + duplicates))))) + +(define (assert-unique-group-names groups) + (match (find-duplicates (map user-group-name groups)) + (() *unspecified*) + (duplicates + (raise + (formatted-message + (G_ "the following groups appear more than once:~{ ~a~}~%") + duplicates))))) + (define (assert-valid-users/groups users groups) "Raise an error if USERS refer to groups not listed in GROUPS." (let ((groups (list->set (map user-group-name groups)))) @@ -292,6 +328,8 @@ group." (define group-specs (map user-group->gexp groups)) + (assert-unique-account-names accounts) + (assert-unique-group-names groups) (assert-valid-users/groups accounts groups) ;; Add users and user groups. -- 2.30.0
Hi, Leo Prikler <leo.prikler@student.tugraz.at> skribis: > Am Mittwoch, den 06.01.2021, 14:32 +0100 schrieb Ludovic Courtès: >> Hi, >> >> Leo Prikler <leo.prikler@student.tugraz.at> skribis: >> >> > > > + ((first . rest) >> > > > + (if (member first rest =) ; (srfi srfi-1) member >> > > > + (cons first (find-duplicates rest =)) >> > > > + (find-duplicates rest =))))) >> > > >> > > Note that this is quadratic; it’s fine as long as we don’t have >> > > “too >> > > many” users, which may be the case in general. >> > It is indeed quadratic, but would there even be an n log n >> > solution? >> > I've once done an n log n sort+delete-duplicates!, perhaps that'd >> > be a >> > nicer solution here? >> >> You could first build a hash table or vhash or set with all the >> names, >> then traverse again the list of names and check whether they’re in >> that >> table. That’d be linear (assuming the table is well balanced), but >> the >> constant factor would be higher. > Yeah, I think the hash table solution would make the most sense here. > Since VHashes are based on VLists, they're not actually purely > functional, are they? Their implementation is not “purely functional” but it’s inconsequential; it’s a persistent data structure, and that’s what matters (info "(guile) VLists"). >> > > (define (assert-unique-account-names users) >> > > (match (find-duplicates things …) >> > > (() #t) >> > > (lst >> > > (raise (formatted-message (G_ "the following accounts >> > > appear >> > > more than once:~{ ~a~}~%" >> > > lst)))))) >> > > >> > > ? >> > That'd be weird for duplicate duplicates, hence just reporting the >> > first. >> >> You could do (delete-duplicates lst) in the message above? > Sure, but that'd be O(n^2) on top of O(n^2), which is less than ideal. Yes, but it’s a small ‘n’, typically one or two. Ludo’.
Leo Prikler <leo.prikler@student.tugraz.at> skribis: > *gnu/system/shadow.scm (find-duplicates): New variable. > (assert-unique-account-names, assert-unique-group-names): New variables. > (account-activation): Use them here. Final nitpicks! :-) > +(define (find-duplicates list) Please add a docstring. > + (let loop ((table (make-hash-table)) > + (list list)) You can move ‘table’ out of the ‘loop’ arguments since it’s mutated anyway. OK with these changes! Thanks, Ludo’.
*gnu/system/shadow.scm (find-duplicates): New variable. (assert-unique-account-names, assert-unique-group-names): New variables. (account-activation): Use them here. --- gnu/system/shadow.scm | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm index a69339bc07..183b2cd387 100644 --- a/gnu/system/shadow.scm +++ b/gnu/system/shadow.scm @@ -20,6 +20,7 @@ ;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>. (define-module (gnu system shadow) + #:use-module ((guix diagnostics) #:select (formatted-message)) #:use-module (guix records) #:use-module (guix gexp) #:use-module (guix store) @@ -34,6 +35,7 @@ #:use-module ((gnu packages admin) #:select (shadow)) #:use-module (gnu packages bash) + #:use-module (ice-9 match) #:use-module (srfi srfi-1) #:use-module (srfi srfi-26) #:use-module (srfi srfi-34) @@ -222,6 +224,46 @@ for a colorful Guile experience.\\n\\n\"))))\n")) (rename-file ".nanorc" ".config/nano/nanorc")) #t)))) +(define (find-duplicates list) + "Find duplicate entries in @var{list}. +Two entries are considered duplicates, if they are @code{equal?} to each other. +This implementation is made asymptotically faster than @code{delete-duplicates} +through the internal use of hash tables." + (let loop ((list list) + ;; We actually modify table in-place, but still allocate it here + ;; so that we only need one level of indentation. + (table (make-hash-table))) + (match list + (() + (hash-fold (lambda (key value seed) + (if (> value 1) + (cons key seed) + seed)) + '() + table)) + ((first . rest) + (hash-set! table first + (1+ (hash-ref table first 0))) + (loop rest table))))) + +(define (assert-unique-account-names users) + (match (find-duplicates (map user-account-name users)) + (() *unspecified*) + (duplicates + (raise + (formatted-message + (G_ "the following accounts appear more than once:~{ ~a~}") + duplicates))))) + +(define (assert-unique-group-names groups) + (match (find-duplicates (map user-group-name groups)) + (() *unspecified*) + (duplicates + (raise + (formatted-message + (G_ "the following groups appear more than once:~{ ~a~}") + duplicates))))) + (define (assert-valid-users/groups users groups) "Raise an error if USERS refer to groups not listed in GROUPS." (let ((groups (list->set (map user-group-name groups)))) @@ -292,6 +334,8 @@ group." (define group-specs (map user-group->gexp groups)) + (assert-unique-account-names accounts) + (assert-unique-group-names groups) (assert-valid-users/groups accounts groups) ;; Add users and user groups. -- 2.30.0
Am Donnerstag, den 07.01.2021, 09:35 +0100 schrieb Ludovic Courtès: > Leo Prikler <leo.prikler@student.tugraz.at> skribis: > > > *gnu/system/shadow.scm (find-duplicates): New variable. > > (assert-unique-account-names, assert-unique-group-names): New > > variables. > > (account-activation): Use them here. > > Final nitpicks! :-) > > > +(define (find-duplicates list) > > Please add a docstring. Done, see v3. > > + (let loop ((table (make-hash-table)) > > + (list list)) > > You can move ‘table’ out of the ‘loop’ arguments since it’s mutated > anyway. I don't see any benefit from doing so, however. It'd be an additional layer of mutation and if we ever wanted to change to vhashes or alists we'd have to refactor that. Regards, Leo
Hi,
Leo Prikler <leo.prikler@student.tugraz.at> skribis:
> *gnu/system/shadow.scm (find-duplicates): New variable.
> (assert-unique-account-names, assert-unique-group-names): New variables.
> (account-activation): Use them here.
LGTM, thanks! :-)
Ludo’.
Am Montag, den 11.01.2021, 14:09 +0100 schrieb Ludovic Courtès:
> Hi,
>
> Leo Prikler <leo.prikler@student.tugraz.at> skribis:
>
> > *gnu/system/shadow.scm (find-duplicates): New variable.
> > (assert-unique-account-names, assert-unique-group-names): New
> > variables.
> > (account-activation): Use them here.
>
> LGTM, thanks! :-)
>
> Ludo’.
Aaaand it's pushed.