Patchwork gnu: Allow nfs file systems to be automatically mounted.

login
register
mail settings
Submitter John Darrington
Date Nov. 26, 2016, 9:36 a.m.
Message ID <1480152990-7080-1-git-send-email-jmd@gnu.org>
Download mbox | patch
Permalink /patch/17948/
State New
Headers show

Comments

John Darrington - Nov. 26, 2016, 9:36 a.m.
* gnu/build/file-systems.scm (mount-file-system): Append target addr= when
mounting nfs filesystems.
---
 gnu/build/file-systems.scm | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)
Ludovic Courtès - Nov. 26, 2016, 6:37 p.m.
John Darrington <jmd@gnu.org> skribis:

> * gnu/build/file-systems.scm (mount-file-system): Append target addr= when
> mounting nfs filesystems.

Looks like you forgot to address some of suggestions I made
(‘string-prefix?’, ‘match’, etc.) and left a question unanswered (port
number?):

  https://lists.gnu.org/archive/html/guix-devel/2016-11/msg00949.html

TIA!

Ludo’.
John Darrington - Nov. 26, 2016, 7:33 p.m.
On Sat, Nov 26, 2016 at 07:37:15PM +0100, Ludovic Court??s wrote:
     John Darrington <jmd@gnu.org> skribis:
     
     > * gnu/build/file-systems.scm (mount-file-system): Append target addr= when
     > mounting nfs filesystems.
     
     Looks like you forgot to address some of suggestions I made
     (???string-prefix????, 

Ok.  I will make that change.

     ???match???, 

Have another look: +    (let* ((host (match (string-split source #\:) ((h _) h)))


     and left a question unanswered (port number?):

Sorry.  The answer is, no so far as I'm aware, there is no convention which 
allows a port number to  be specifed in the source field.

     , etc)

The only other thing I think you mentioned was using "if" instead of "cond".
I can envisage that in the future there will be a need for other cases,  - 
it's not just an either/or choice - so it seemed to me that cond was more 
appropriate.
     
I am aware there is room for improvement in this patch (for example in the 
case of IP6 addresses?)  - any help will be gratefully accepted.

J'
Ludovic Courtès - Nov. 28, 2016, 1:59 p.m.
John Darrington <john@darrington.wattle.id.au> skribis:

> On Sat, Nov 26, 2016 at 07:37:15PM +0100, Ludovic Court??s wrote:
>      John Darrington <jmd@gnu.org> skribis:
>      
>      > * gnu/build/file-systems.scm (mount-file-system): Append target addr= when
>      > mounting nfs filesystems.
>      
>      Looks like you forgot to address some of suggestions I made
>      (???string-prefix????, 
>
> Ok.  I will make that change.
>
>      ???match???, 
>
> Have another look: +    (let* ((host (match (string-split source #\:) ((h _) h)))

Right, but there are other occurrences of ‘car’ for ‘getaddrinfo’.  :-)

>      and left a question unanswered (port number?):
>
> Sorry.  The answer is, no so far as I'm aware, there is no convention which 
> allows a port number to  be specifed in the source field.

OK (this would have changed the way ‘source’ is parsed.)

>      , etc)
>
> The only other thing I think you mentioned was using "if" instead of "cond".
> I can envisage that in the future there will be a need for other cases,  - 
> it's not just an either/or choice - so it seemed to me that cond was more 
> appropriate.
>      
> I am aware there is room for improvement in this patch (for example in the 
> case of IP6 addresses?)  - any help will be gratefully accepted.

‘getaddrinfo’ can return both IPv4 and IPv6 addresses, so in theory,
the patch is IPv6 Ready.

Ludo’.
John Darrington - Nov. 28, 2016, 2:07 p.m.
On Mon, Nov 28, 2016 at 02:59:09PM +0100, Ludovic Court??s wrote:
     >      ???match???, 
     >
     > Have another look: +    (let* ((host (match (string-split source #\:) ((h _) h)))
     
     Right, but there are other occurrences of ???car??? for ???getaddrinfo???.  :-)

But that occurance applies to a real list, rather than a list used as record.
In other words, I really do just want to get the first item of that list.
As I understand it, match is supposed to be used for heterogenous lists where
each member has its own semantics.  That is not the case here.

Obviously, I could write (match ... (x . _) x) but I don't see what benefit that
brings.
     

J'
Ludovic Courtès - Nov. 28, 2016, 9:05 p.m.
John Darrington <john@darrington.wattle.id.au> skribis:

> On Mon, Nov 28, 2016 at 02:59:09PM +0100, Ludovic Court??s wrote:
>      >      ???match???, 
>      >
>      > Have another look: +    (let* ((host (match (string-split source #\:) ((h _) h)))
>      
>      Right, but there are other occurrences of ???car??? for ???getaddrinfo???.  :-)
>
> But that occurance applies to a real list, rather than a list used as record.
> In other words, I really do just want to get the first item of that list.
> As I understand it, match is supposed to be used for heterogenous lists where
> each member has its own semantics.  That is not the case here.

‘match’ can be used to match anything, and I highly recommend using it
for lists (info "(guile) Pairs"): it generates clearer and foolproof
code.

In this case ‘getaddrinfo’ might well return an empty list.

Ludo’.
John Darrington - Nov. 29, 2016, 6:27 a.m.
On Mon, Nov 28, 2016 at 10:05:15PM +0100, Ludovic Court??s wrote:
     John Darrington <john@darrington.wattle.id.au> skribis:
     
     > On Mon, Nov 28, 2016 at 02:59:09PM +0100, Ludovic Court??s wrote:
     >      >      ???match???, 
     >      >
     >      > Have another look: +    (let* ((host (match (string-split source #\:) ((h _) h)))
     >      
     >      Right, but there are other occurrences of ???car??? for ???getaddrinfo???.  :-)
     >
     > But that occurance applies to a real list, rather than a list used as record.
     > In other words, I really do just want to get the first item of that list.
     > As I understand it, match is supposed to be used for heterogenous lists where
     > each member has its own semantics.  That is not the case here.
     
     ???match??? can be used to match anything, and I highly recommend using it
     for lists (info "(guile) Pairs"): it generates clearer and foolproof
     code.
     
     In this case ???getaddrinfo??? might well return an empty list.
     
So then "car" would raise an error (unless I check it first with pair?) - just
as match would raise an error unless I add a catch-all case.  I'm still not 
convinced that match has any advantages in this case.

However I've changed it despite that, and pushed this patch to master.  I hope
it's ok.

J'
Ludovic Courtès - Nov. 29, 2016, 9:51 p.m.
John Darrington <john@darrington.wattle.id.au> skribis:

> On Mon, Nov 28, 2016 at 10:05:15PM +0100, Ludovic Court??s wrote:
>      John Darrington <john@darrington.wattle.id.au> skribis:
>      
>      > On Mon, Nov 28, 2016 at 02:59:09PM +0100, Ludovic Court??s wrote:
>      >      >      ???match???, 
>      >      >
>      >      > Have another look: +    (let* ((host (match (string-split source #\:) ((h _) h)))
>      >      
>      >      Right, but there are other occurrences of ???car??? for ???getaddrinfo???.  :-)
>      >
>      > But that occurance applies to a real list, rather than a list used as record.
>      > In other words, I really do just want to get the first item of that list.
>      > As I understand it, match is supposed to be used for heterogenous lists where
>      > each member has its own semantics.  That is not the case here.
>      
>      ???match??? can be used to match anything, and I highly recommend using it
>      for lists (info "(guile) Pairs"): it generates clearer and foolproof
>      code.
>      
>      In this case ???getaddrinfo??? might well return an empty list.
>      
> So then "car" would raise an error (unless I check it first with pair?) - just
> as match would raise an error unless I add a catch-all case.  I'm still not 
> convinced that match has any advantages in this case.
>
> However I've changed it despite that, and pushed this patch to master.  I hope
> it's ok.

No big deal, but it’s not what I meant.

An expression like:

  (let ((x (match lst1 ((a . b) a)))
        (y (match lst2 ((a . b) a))))
    …)

is almost equivalent to using ‘car’ twice, only more verbose.  That is,
the cases where ‘lst1’ or ‘lst2’ is empty are not handled (‘match’
raises an exception.)

The real gain is when each case is explicitly separated and handled:

  (match lst1
    ((x . _)
     (match lst2
       ((y . _)
        (do-something x y))
       (_
        (error "lst2 is empty"))))
    (_
     (error "lst1 is bogus")))

In this case we’d want to gracefully handle syntax errors in ‘source’
and host name lookup failures for instance.

Ludo’.

Patch

diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
index c6fc784..ca788ec 100644
--- a/gnu/build/file-systems.scm
+++ b/gnu/build/file-systems.scm
@@ -464,6 +464,22 @@  form:
 DEVICE, MOUNT-POINT, and TYPE must be strings; OPTIONS can be a string or #f;
 FLAGS must be a list of symbols.  CHECK? is a Boolean indicating whether to
 run a file system check."
+
+  (define (mount-nfs source mount-point type flags options)
+    (let* ((host (match (string-split source #\:) ((h _) h)))
+           (aa (car (getaddrinfo host "nfs")))
+           (sa (addrinfo:addr aa))
+           (inet-addr (inet-ntop (sockaddr:fam sa)
+                                 (sockaddr:addr sa))))
+
+      ;; Mounting an NFS file system requires passing the address
+      ;; of the server in the addr= option
+      (mount source mount-point type flags
+             (string-append "addr="
+                            inet-addr
+                            (if options
+                                (string-append "," options)
+                                "")))))
   (match spec
     ((source title mount-point type (flags ...) options check?)
      (let ((source      (canonicalize-device-spec source title))
@@ -481,21 +497,11 @@  run a file system check."
              (call-with-output-file mount-point (const #t)))
            (mkdir-p mount-point))
 
-       (mount source mount-point type flags
-              (cond
-               ((string-match "^nfs.*" type)
-                (let* ((host (car (string-split source #\:)))
-                       (aa (car (getaddrinfo host #f)))
-                       (sa (addrinfo:addr aa))
-                       (inet-addr (inet-ntop (sockaddr:fam sa)
-                                             (sockaddr:addr sa))))
-                  (string-append "addr="
-                                 inet-addr
-                                 (if options
-                                     (string-append "," options)
-                                     ""))))
-               (else
-                options)))
+       (cond
+        ((string-match "^nfs.*" type)
+         (mount-nfs source mount-point type flags options))
+        (else
+         (mount source mount-point type flags options)))
 
        ;; For read-only bind mounts, an extra remount is needed, as per
        ;; <http://lwn.net/Articles/281157/>, which still applies to Linux 4.0.