bug#22050: [PATCH v4 1/2] linux-boot: Add make-static-device-nodes.

[ludo@gnu.org (Ludovic Courtès)]

Hi Danny,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> * gnu/build/linux-boot.scm (make-static-device-nodes): New variable.
> <device-node>: New variable.
> parse-static-nodes-from-devname-file: New variable.
> not-slash: New variable.
> report-system-error: New variable.
> catch-system-error: New variable.
> create-device-node: New variable.

Nitpick: please adjust the syntax.

> +(define-record-type <device-node>
> +  (device-node name type major minor module)

Please add a one-line comment above like:

  ;; Representation of a /dev node.

> +(define (parse-static-nodes-from-devname-file devname-name)
> +  (call-with-input-file devname-name
> +    (lambda (input-file)

It would be more idiomatic to take an input port, rather than a file
name.  Also I’d suggest ‘read-static-device-nodes’, with a docstring:

  (define (read-static-device-nodes port)
    "Read from PORT a list of <device-node> written in the format used
  by /lib/modules/*/*.devname files."
    …)

> +      (let loop ((line (read-line input-file)))
> +        (if (eof-object? line)
> +          '()
> +          (match (string-split line #\space)
> +           (("#" _ ...)
> +            (loop (read-line input-file)))

To make sure all comments are handled, change this clause to:

  (((? (cut string-prefix? "#" <>)) _ ...)
   (loop (read-line input-line)))

> +(define (report-system-error name . args)
> +  (let ((errno (system-error-errno args)))
> +        (format (current-error-port) "could not create '~a': ~a~%" name
> +                (strerror errno))))

Align “(format” with the ‘e’ of ‘let’.  :-)

> +(define create-device-node

Please add a comment saying what it does.

> +  (match-lambda
> +    (($ <device-node> name type major minor module)
> +     (let ((name-parts (string-tokenize name not-slash)))
> +       (let loop ((prefix "/dev")
> +                  (name-parts name-parts))
> +         (match name-parts
> +          ((leaf)
> +           (let ((prefix (string-append prefix "/" leaf)))
> +             (catch-system-error prefix
> +               (mknod prefix type #o600 (device-number major minor)))))
> +          ((prefix-addition tails ...)
> +           (let ((prefix (string-append prefix "/" prefix-addition)))
> +             (unless (file-exists? prefix)
> +               (mkdir prefix #o755))
> +             (loop prefix tails)))))))))

This looks good, but would it be enough to do:

  (mkdir-p (dirname (string-append "/dev/" name)))

?

> +(define* (make-static-device-nodes linux-module-directory)

Docstring please.  :-)  IMO it’s important also to mention why those
nodes need to be created by hand.

Thank you!

Ludo’.