From 3c5fca9618241334f40bfd2199cdfabb4fad55ec Mon Sep 17 00:00:00 2001
From: Aneesh Agrawal <aneeshusa@gmail.com>
Date: Wed, 21 Oct 2015 17:37:14 +0000
Subject: [PATCH] filesystems: use list of strings for fs options

Allow usage of list of strings instead of a comma-separated string
for filesystem options. Deprecate the comma-separated string style
with a warning message; convert this to a hard error after 16.09.
15.09 was just released, so this provides a deprecation period during
the 16.03 release.

closes #10518

Signed-off-by: Robin Gloster <mail@glob.in>
---
 nixos/doc/manual/configuration/config-file.xml |  2 +-
 nixos/doc/manual/man-nixos-generate-config.xml |  4 ++--
 nixos/doc/manual/release-notes/rl-unstable.xml | 17 +++++++++++++++++
 nixos/modules/installer/cd-dvd/iso-image.nix   |  8 ++++----
 .../installer/tools/nixos-generate-config.pl   |  4 ++--
 nixos/modules/system/boot/stage-1.nix          |  2 +-
 nixos/modules/tasks/filesystems.nix            | 18 +++++++++++-------
 nixos/modules/virtualisation/qemu-vm.nix       | 12 ++++++------
 nixos/tests/misc.nix                           |  2 +-
 nixos/tests/nfs.nix                            |  2 +-
 10 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/nixos/doc/manual/configuration/config-file.xml b/nixos/doc/manual/configuration/config-file.xml
index b613c7f06cc8..9b240979273d 100644
--- a/nixos/doc/manual/configuration/config-file.xml
+++ b/nixos/doc/manual/configuration/config-file.xml
@@ -157,7 +157,7 @@ boot.kernel.sysctl."net.ipv4.tcp_keepalive_time" = 60;
 fileSystems."/boot" =
   { device = "/dev/sda1";
     fsType = "ext4";
-    options = "rw,data=ordered,relatime";
+    options = [ "rw" "data=ordered" "relatime" ];
   };
 </programlisting>
       </para>
diff --git a/nixos/doc/manual/man-nixos-generate-config.xml b/nixos/doc/manual/man-nixos-generate-config.xml
index e4fba4a40a97..140642bc9c9c 100644
--- a/nixos/doc/manual/man-nixos-generate-config.xml
+++ b/nixos/doc/manual/man-nixos-generate-config.xml
@@ -165,13 +165,13 @@ look like this:
   fileSystems."/" =
     { device = "/dev/disk/by-label/nixos";
       fsType = "ext3";
-      options = "rw,data=ordered,relatime";
+      options = [ "rw" "data=ordered" "relatime" ];
     };
 
   fileSystems."/boot" =
     { device = "/dev/sda1";
       fsType = "ext3";
-      options = "rw,errors=continue,user_xattr,acl,barrier=1,data=writeback,relatime";
+      options = [ "rw" "errors=continue" "user_xattr" "acl" "barrier=1" "data=writeback" "relatime" ];
     };
 
   swapDevices =
diff --git a/nixos/doc/manual/release-notes/rl-unstable.xml b/nixos/doc/manual/release-notes/rl-unstable.xml
index 3d7819803697..c814d61bcf4c 100644
--- a/nixos/doc/manual/release-notes/rl-unstable.xml
+++ b/nixos/doc/manual/release-notes/rl-unstable.xml
@@ -155,6 +155,23 @@ nginx.override {
     rights, new <literal>aliasFiles</literal> and <literal>mapFiles</literal>
     options and more.</para>
   </listitem>
+
+  <listitem>
+    <para>Filesystem options should now be configured as a list of strings, not
+    a comma-separated string. The old style will continue to work, but print a
+    warning, until the 16.09 release. An example of the new style:
+
+<programlisting>
+fileSystems."/example" = {
+  device = "/dev/sdc";
+  fsType = "btrfs";
+  options = [ "noatime" "compress=lzo" "space_cache" "autodefrag" ];
+};
+</programlisting>
+
+    </para>
+  </listitem>
+
 </itemizedlist>
 
 
diff --git a/nixos/modules/installer/cd-dvd/iso-image.nix b/nixos/modules/installer/cd-dvd/iso-image.nix
index 248b0f00283c..5702e2d9a1e5 100644
--- a/nixos/modules/installer/cd-dvd/iso-image.nix
+++ b/nixos/modules/installer/cd-dvd/iso-image.nix
@@ -249,7 +249,7 @@ in
 
     fileSystems."/" =
       { fsType = "tmpfs";
-        options = "mode=0755";
+        options = [ "mode=0755" ];
       };
 
     # Note that /dev/root is a symlink to the actual root device
@@ -266,20 +266,20 @@ in
     fileSystems."/nix/.ro-store" =
       { fsType = "squashfs";
         device = "/iso/nix-store.squashfs";
-        options = "loop";
+        options = [ "loop" ];
         neededForBoot = true;
       };
 
     fileSystems."/nix/.rw-store" =
       { fsType = "tmpfs";
-        options = "mode=0755";
+        options = [ "mode=0755" ];
         neededForBoot = true;
       };
 
     fileSystems."/nix/store" =
       { fsType = "unionfs-fuse";
         device = "unionfs";
-        options = "allow_other,cow,nonempty,chroot=/mnt-root,max_files=32768,hide_meta_files,dirs=/nix/.rw-store=rw:/nix/.ro-store=ro";
+        options = [ "allow_other" "cow" "nonempty" "chroot=/mnt-root" "max_files=32768" "hide_meta_files" "dirs=/nix/.rw-store=rw:/nix/.ro-store=ro" ];
       };
 
     boot.initrd.availableKernelModules = [ "squashfs" "iso9660" "usb-storage" ];
diff --git a/nixos/modules/installer/tools/nixos-generate-config.pl b/nixos/modules/installer/tools/nixos-generate-config.pl
index 2284eba07a2d..ec880e084726 100644
--- a/nixos/modules/installer/tools/nixos-generate-config.pl
+++ b/nixos/modules/installer/tools/nixos-generate-config.pl
@@ -349,7 +349,7 @@ foreach my $fs (read_file("/proc/self/mountinfo")) {
   fileSystems.\"$mountPoint\" =
     { device = \"$base$path\";
       fsType = \"none\";
-      options = \"bind\";
+      options = \[ \"bind\" \];
     };
 
 EOF
@@ -409,7 +409,7 @@ EOF
 
     if (scalar @extraOptions > 0) {
       $fileSystems .= <<EOF;
-      options = \"${\join ",", uniq(@extraOptions)}\";
+      options = \[ ${\join " ", map { "\"" . $_ . "\"" } uniq(@extraOptions)} \];
 EOF
     }
 
diff --git a/nixos/modules/system/boot/stage-1.nix b/nixos/modules/system/boot/stage-1.nix
index 1d9a67abcf9e..f31620df1d85 100644
--- a/nixos/modules/system/boot/stage-1.nix
+++ b/nixos/modules/system/boot/stage-1.nix
@@ -207,7 +207,7 @@ let
                     (filter (sd: (sd ? label || hasPrefix "/dev/" sd.device) && !sd.randomEncryption) config.swapDevices);
 
     fsInfo =
-      let f = fs: [ fs.mountPoint (if fs.device != null then fs.device else "/dev/disk/by-label/${fs.label}") fs.fsType fs.options ];
+      let f = fs: [ fs.mountPoint (if fs.device != null then fs.device else "/dev/disk/by-label/${fs.label}") fs.fsType (builtins.concatStringsSep "," fs.options) ];
       in pkgs.writeText "initrd-fsinfo" (concatStringsSep "\n" (concatMap f fileSystems));
 
     setHostId = optionalString (config.networking.hostId != null) ''
diff --git a/nixos/modules/tasks/filesystems.nix b/nixos/modules/tasks/filesystems.nix
index d0dd7670157e..4d1466db22d0 100644
--- a/nixos/modules/tasks/filesystems.nix
+++ b/nixos/modules/tasks/filesystems.nix
@@ -41,11 +41,15 @@ let
       };
 
       options = mkOption {
-        default = "defaults";
-        example = "data=journal";
-        type = types.commas; # FIXME: should be a list
+        default = [ "defaults" ];
+        example = [ "data=journal" ];
         description = "Options used to mount the file system.";
-      };
+      } // (if versionAtLeast lib.nixpkgsVersion "16.09" then {
+        type = types.listOf types.str;
+      } else {
+        type = types.either types.commas (types.listOf types.str);
+        apply = x: if isList x then x else lib.strings.splitString "," (builtins.trace "warning: passing a comma-separated string for filesystem options is deprecated; use a list of strings instead. This will become a hard error in 16.09." x);
+      });
 
       autoFormat = mkOption {
         default = false;
@@ -112,7 +116,7 @@ in
         "/data" = {
           device = "/dev/hda2";
           fsType = "ext3";
-          options = "data=journal";
+          options = [ "data=journal" ];
         };
         "/bigdisk".label = "bigdisk";
       };
@@ -127,7 +131,7 @@ in
         <command>mount</command>; defaults to
         <literal>"auto"</literal>), and <literal>options</literal>
         (the mount options passed to <command>mount</command> using the
-        <option>-o</option> flag; defaults to <literal>"defaults"</literal>).
+        <option>-o</option> flag; defaults to <literal>[ "defaults" ]</literal>).
 
         Instead of specifying <literal>device</literal>, you can also
         specify a volume label (<literal>label</literal>) for file
@@ -177,7 +181,7 @@ in
              else throw "No device specified for mount point ‘${fs.mountPoint}’.")
             + " " + fs.mountPoint
             + " " + fs.fsType
-            + " " + fs.options
+            + " " + builtins.concatStringsSep "," fs.options
             + " 0"
             + " " + (if skipCheck fs then "0" else
                      if fs.mountPoint == "/" then "1" else "2")
diff --git a/nixos/modules/virtualisation/qemu-vm.nix b/nixos/modules/virtualisation/qemu-vm.nix
index 82b58aa67a3d..6e0dc065387a 100644
--- a/nixos/modules/virtualisation/qemu-vm.nix
+++ b/nixos/modules/virtualisation/qemu-vm.nix
@@ -427,38 +427,38 @@ in
         ${if cfg.writableStore then "/nix/.ro-store" else "/nix/store"} =
           { device = "store";
             fsType = "9p";
-            options = "trans=virtio,version=9p2000.L,cache=loose";
+            options = [ "trans=virtio" "version=9p2000.L" "cache=loose" ];
             neededForBoot = true;
           };
         "/tmp/xchg" =
           { device = "xchg";
             fsType = "9p";
-            options = "trans=virtio,version=9p2000.L,cache=loose";
+            options = [ "trans=virtio" "version=9p2000.L" "cache=loose" ];
             neededForBoot = true;
           };
         "/tmp/shared" =
           { device = "shared";
             fsType = "9p";
-            options = "trans=virtio,version=9p2000.L";
+            options = [ "trans=virtio" "version=9p2000.L" ];
             neededForBoot = true;
           };
       } // optionalAttrs cfg.writableStore
       { "/nix/store" =
           { fsType = "unionfs-fuse";
             device = "unionfs";
-            options = "allow_other,cow,nonempty,chroot=/mnt-root,max_files=32768,hide_meta_files,dirs=/nix/.rw-store=rw:/nix/.ro-store=ro";
+            options = [ "allow_other" "cow" "nonempty" "chroot=/mnt-root" "max_files=32768" "hide_meta_files" "dirs=/nix/.rw-store=rw:/nix/.ro-store=ro" ];
           };
       } // optionalAttrs (cfg.writableStore && cfg.writableStoreUseTmpfs)
       { "/nix/.rw-store" =
           { fsType = "tmpfs";
-            options = "mode=0755";
+            options = [ "mode=0755" ];
             neededForBoot = true;
           };
       } // optionalAttrs cfg.useBootLoader
       { "/boot" =
           { device = "/dev/vdb2";
             fsType = "vfat";
-            options = "ro";
+            options = [ "ro" ];
             noCheck = true; # fsck fails on a r/o filesystem
           };
       });
diff --git a/nixos/tests/misc.nix b/nixos/tests/misc.nix
index 6297452df95e..73af0cfad21f 100644
--- a/nixos/tests/misc.nix
+++ b/nixos/tests/misc.nix
@@ -16,7 +16,7 @@ import ./make-test.nix ({ pkgs, ...} : {
       systemd.tmpfiles.rules = [ "d /tmp 1777 root root 10d" ];
       fileSystems = mkVMOverride { "/tmp2" =
         { fsType = "tmpfs";
-          options = "mode=1777,noauto";
+          options = [ "mode=1777" "noauto" ];
         };
       };
       systemd.automounts = singleton
diff --git a/nixos/tests/nfs.nix b/nixos/tests/nfs.nix
index 24f6e0f2ed95..36cd6a395779 100644
--- a/nixos/tests/nfs.nix
+++ b/nixos/tests/nfs.nix
@@ -8,7 +8,7 @@ let
         [ { mountPoint = "/data";
             device = "server:/data";
             fsType = "nfs";
-            options = "vers=${toString version}";
+            options = [ "vers=${toString version}" ];
           }
         ];
       networking.firewall.enable = false; # FIXME: only open statd