[bug#70542,3/4] file-systems: Add support for mounting CIFS file systems

* gnu/build/file-systems (canonicalize-device-name): Do not attempt to resolve
CIFS formatted device specifications.
* gnu/build/file-systems (mount-file-system): Add (mount-cifs)
and (host-to-ip). Logic for ip/host to ip resolution was duplicated with
mount-nfs, so isolate into a dedicated function.

Change-Id: I522d70a10651ca79533a4fc60b96b884243a3526
---
 gnu/build/file-systems.scm | 60 +++++++++++++++++++++++++++++++++-----
 1 file changed, 53 insertions(+), 7 deletions(-)

Am Dienstag, dem 23.04.2024 um 16:47 -0400 schrieb Richard Sent:
> * gnu/build/file-systems (canonicalize-device-name): Do not attempt
> to resolve CIFS formatted device specifications.
> * gnu/build/file-systems (mount-file-system): Add (mount-cifs)
> and (host-to-ip). Logic for ip/host to ip resolution was duplicated
> with mount-nfs, so isolate into a dedicated function.
You should factor out host-to-ip in a separate patch before adding
mount-cifs.

> Change-Id: I522d70a10651ca79533a4fc60b96b884243a3526
> ---
>  gnu/build/file-systems.scm | 60 +++++++++++++++++++++++++++++++++---
> --
>  1 file changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
> index 78d779f398..ae29b36c4e 100644
> --- a/gnu/build/file-systems.scm
> +++ b/gnu/build/file-systems.scm
> @@ -8,6 +8,7 @@
>  ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
>  ;;; Copyright © 2022 Oleg Pykhalov <go.wigust@gmail.com>
>  ;;; Copyright © 2024 Nicolas Graves <ngraves@ngraves.fr>
> +;;; Copyright © 2024 Richard Sent <richard@freakingpenguin.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -37,6 +38,7 @@ (define-module (gnu build file-systems)
>    #:use-module (rnrs bytevectors)
>    #:use-module (ice-9 match)
>    #:use-module (ice-9 rdelim)
> +  #:use-module (ice-9 regex)
>    #:use-module (system foreign)
>    #:autoload   (system repl repl) (start-repl)
>    #:use-module (srfi srfi-1)
> @@ -1047,8 +1049,11 @@ (define (canonicalize-device-spec spec)
>  
>    (match spec
>      ((? string?)
> -     (if (or (string-contains spec ":/") (string=? spec "none"))
> -         spec                  ; do not resolve NFS / tmpfs devices
> +     (if (or (string-contains spec ":/") ;nfs
> +             (and (>= (string-length spec) 2)
> +                  (equal? (string-take spec 2) "//")) ;cifs
> +             (string=? spec "none"))
> +         spec                  ; do not resolve NFS / CIFS / tmpfs
> devices
>           ;; Nothing to do, but wait until SPEC shows up.
>           (resolve identity spec identity)))
>      ((? file-system-label?)
> @@ -1156,6 +1161,14 @@ (define* (mount-file-system fs #:key (root
> "/root")
>                              (repair (file-system-repair fs)))
>    "Mount the file system described by FS, a <file-system> object,
> under ROOT."
>  
> +  (define* (host-to-ip host #:optional service)
> +    "Return the IP address for host, which may be an IP address or a
> hostname."
> +    (let* ((aa (match (getaddrinfo host service) ((x . _) x)))
> +           (sa (addrinfo:addr aa))
> +           (inet-addr (inet-ntop (sockaddr:fam sa)
> +                                 (sockaddr:addr sa))))
> +      inet-addr))
> +
>    (define (mount-nfs source mount-point type flags options)
>      (let* ((idx (string-rindex source #\:))
>             (host-part (string-take source idx))
> @@ -1163,11 +1176,7 @@ (define* (mount-file-system fs #:key (root
> "/root")
>             (host (match (string-split host-part (string->char-set
> "[]"))
>                   (("" h "") h)
>                   ((h) h)))
> -           (aa (match (getaddrinfo host "nfs") ((x . _) x)))
> -           (sa (addrinfo:addr aa))
> -           (inet-addr (inet-ntop (sockaddr:fam sa)
> -                                 (sockaddr:addr sa))))
> -
> +           (inet-addr (host-to-ip host "nfs")))
>        ;; Mounting an NFS file system requires passing the address
>        ;; of the server in the addr= option
>        (mount source mount-point type flags
> @@ -1176,6 +1185,41 @@ (define* (mount-file-system fs #:key (root
> "/root")
>                              (if options
>                                  (string-append "," options)
>                                  "")))))
> +
> +  (define (mount-cifs source mount-point type flags options)
> +    ;; Source is of form "//<server-ip-or-host>/<service>"
> +    (let* ((regex-match (string-match "//([^/]+)/(.+)" source))
> +           (server (match:substring regex-match 1))
> +           (share (match:substring regex-match 2))
> +           ;; Match ",guest,", ",guest$", "^guest,", or "^guest$,"
> not
> +           ;; e.g. user=foo,pass=notaguest
> +           (guest? (string-match "(^|,)(guest)($|,)" options))
> +           ;; Perform DNS resolution now instead of attempting
> kernel dns
> +           ;; resolver upcalling. /sbin/request-key does not exist
> and the
> +           ;; kernel hardcodes the path.
> +           ;;
> +           ;; (getaddrinfo) doesn't support cifs service, so omit
> it.
> +           (inet-addr (host-to-ip server)))
> +      (mount source mount-point type flags
> +             (string-append "ip="
> +                            inet-addr
> +                            ;; As of Linux af1a3d2ba9 (v5.11) unc is
> ignored
> +                            ;; and source is parsed by the kernel
> +                            ;; directly. Pass it for compatibility.
> +                            ",unc="
> +                            ;; Match format of mount.cifs's mount
> syscall.
> +                            "\\\\" server "\\" share
> +                            (if guest?
> +                                ",user=,pass="
> +                                "")
> +                            (if options
> +                                ;; No need to delete "guest" from
> options.
> +                                ;; linux/fs/smb/client/fs_context.c
> explicitly
> +                                ;; ignores it. Also, avoiding excess
> commas
> +                                ;; when deleting is a pain.
> +                                (string-append "," options)
> +                                "")))))
Any reason we ought to solve guest specially?  Let's just assume that
user and pass are always (possibly empty) strings.  If you need to
abstract over it, you could always make a procedure or something.

Cheers

Hi!

Thanks for the feedback! :)

> Any reason we ought to solve guest specially? Let's just assume that
> user and pass are always (possibly empty) strings. If you need to
> abstract over it, you could always make a procedure or something.

My reason for handling guest specifically is to try and match the
behavior of the userspace "mount.cifs" program as much as possible. That
program will parse options, and if it encounters the option "guest",
silently replace it with "user=,pass=" before initiating the mount
system call.

The kernel driver itself ignores the "guest" option, so unless we handle
it ourselves by inserting blank user and pass options, it wouldn't have
an effect.

If I understand what you're saying, we could have user and pass
variables that default to "" unless options includes e.g.
user=foo,pass=bar. That would diverge from mount.cifs's behavior though,
which is something I'm reluctant to do.

I don't know if not passing user or pass is identical to passing
user=,pass=, but if it's not then users would get confused reading
mount.cifs's documentation and trying to apply the same logic to their
OS declaration.

Am Mittwoch, dem 24.04.2024 um 14:22 -0400 schrieb Richard Sent:
> Hi!
> 
> Thanks for the feedback! :)
> 
> > Any reason we ought to solve guest specially? Let's just assume
> > that user and pass are always (possibly empty) strings. If you need
> > to abstract over it, you could always make a procedure or
> > something.
> 
> My reason for handling guest specifically is to try and match the
> behavior of the userspace "mount.cifs" program as much as possible.
> That program will parse options, and if it encounters the option
> "guest", silently replace it with "user=,pass=" before initiating the
> mount system call.
> 
> The kernel driver itself ignores the "guest" option, so unless we
> handle it ourselves by inserting blank user and pass options, it
> wouldn't have an effect.
> 
> If I understand what you're saying, we could have user and pass
> variables that default to "" unless options includes e.g.
> user=foo,pass=bar. That would diverge from mount.cifs's behavior
> though, which is something I'm reluctant to do.
> 
> I don't know if not passing user or pass is identical to passing
> user=,pass=, but if it's not then users would get confused reading
> mount.cifs's documentation and trying to apply the same logic to
> their OS declaration.
I'm rarely that deep in our interaction with file systems, but do we go
through mount or do directly talk to the kernel driver?  If we do
mount.cifs under the hood, this should not be an issue.  Otherwise, we
should do our best to document this behaviour.  I assume that
specifying neither user nor guest would result in an error, but you're
welcome to discover bugs^Wfresh new features.

Cheers

> I'm rarely that deep in our interaction with file systems

Haha, join the club!

> do we go through mount or do directly talk to the kernel driver? If we
> do mount.cifs under the hood, this should not be an issue. Otherwise,
> we should do our best to document this behaviour.

mount-file-system uses the mount system call under the hood, no
userspace tooling as far as I can tell. See mount in (guix build
syscalls). Unfortunately this means we need to replicate the userspace
mount.cifs program's behavior for handling the guest option.

This would definitely be easier (and probably compatible with more file
systems) if we used the standard userspace tools, but I don't think
there's a good way to do that at present.

My hope is that if we match mount.cifs's behavior in how we handle the
guest option (adding user=,pass=), we won't need extra documentation
explaining CIFS file system options. Users would just follow
mount.cifs's documentation (which is probably what they'd find first).
To my knowledge the patch's current behavior w.r.t. guest matches
mount.cifs.

If we were to implement our own divergent behavior from mount.cifs, then
we would need to document that divergence. But I don't think we need to
document that our behavior matches the mount.cifs program's behavior.

(Or, if we do, it should probably be a more general statement like "File
system options are passed to the mount system call, with slight
adjustments to match the userspace mount.<type> equivalent program's
behavior." Something akin to that, but it's not specifically a CIFS
thing.)

> I assume that specifying neither user nor guest would result in an
> error, but you're welcome to discover bugs^Wfresh new features.

I think I'd be content with calling that "user error". If mount.cifs
doesn't do anything special, neither should we. ;)