Merge lp:~wesley-wiedenmeier/curtin/mkfs into lp:~curtin-dev/curtin/trunk

[Wesley Wiedenmeier]

Move mkfs logic into block.mkfs to allow for more flexible support for different format types and unit tests

[Ryan Harper (raharper)]

Thanks for this cleanup. In general looks good. A few comments below.
I'd also like to see some negative unit tests for each place you raise a ValueError exception, you should provide bogus data and self.assertRaises(ValueError, fn) etc.

Also, we know of at least one edge case (btrfs on precise), we should have a test-case specifically for that one. You'll need to mock out the lsb_release call for that. I used:

@patch('curtin.block.mkfs.util')
mock_util.lsb_release.return_value = {'codename': 'precise'}

review: Needs Fixing

[lp:~wesley-wiedenmeier/curtin/mkfs]

336. By Wesley Wiedenmeier on 2016-01-04

Comment typo fixes

337. By Wesley Wiedenmeier on 2016-01-04

Verify that path for curtin.block.mkfs.mkfs is a valid block dev

338. By Wesley Wiedenmeier on 2016-01-04

General cleanup, and comment addition

339. By Wesley Wiedenmeier on 2016-01-04

Renamed curtin.block.mkfs.mkfs to curtin.block.mkfs.run_mkfs and changed import
in curtin.commands.mkfs to avoid name conflict between block.mkfs and
commands.mkfs

340. By Wesley Wiedenmeier on 2016-01-04

Add check for precise + btrfs edge case in mkfs unit tests

341. By Wesley Wiedenmeier on 2016-01-04

Added negative unit tests for all errors that mkfs can raise

[Scott Moser (smoser)]

This looks really good. Some comments in line.
One request is to make curtin/commands/mkfs be a simple wrapper around this.

So we'd then have:
  curtin mkfs --label=FOO /dev/vdb1 ext4
I think I like that better than :
  curtin mkfs /dev/vdb1 ext4 label=FOO uuid=BAR

but I'm not opposed to arguments for either.

[lp:~wesley-wiedenmeier/curtin/mkfs]

342. By Wesley Wiedenmeier on 2016-01-05

Rename block.mkfs.run_mkfs to block.mkfs.mkfs, and use import as in
commands.mkfs to avoid conflict

343. By Wesley Wiedenmeier on 2016-01-05

Switched to long form flags where available

344. By Wesley Wiedenmeier on 2016-01-05

Improve docstring

345. By Wesley Wiedenmeier on 2016-01-05

Check that mkfs command exists on system before trying to run it

346. By Wesley Wiedenmeier on 2016-01-05

Moved negative unit tests for block.mkfs into separate functions for better
readibility/more informative errors

347. By Wesley Wiedenmeier on 2016-01-05

Do not add quiet flag by default, as command output is captured

348. By Wesley Wiedenmeier on 2016-01-05

Removed -q from expected flags

349. By Wesley Wiedenmeier on 2016-01-05

Added strict flag to control mkfs behavior when an invalid format flag is
specified or when label is too long

 - With strict=True, raise error if flag specified that is not supported by
   current fs family, raise error if filesystem label is too long
 - With strict=False, silently ignore flag if not supported by current fs
   family (default behavior of previous code in block_meta) and truncate
   filesystem label if it is too long (many of the mkfs commands do this
   anyway)

[Scott Moser (smoser)]

Looking again at this, it looks like you've addressed most of my concerns.
The remaining things I see are:
1.)
| please improve the docstring here to cover what is in 'flags' at least.
|
| Would it work to give this a signature like:
| run_mkfs(path, fstype, label=None, uuid=None, ...)
|
| then you could call run_mkfs from your 'mkfs_from_config' via:
| flags = {i: info.get(i) for i in info if i in family_flag_mappings)}
| run_mkfs(dev, fstype, **flags)

2.) fix mkfs command to be user usable wrapper around mkfs()
  curtin mkfs --label=FOO /dev/vdb1 ext4
I think I like that better than :
  curtin mkfs /dev/vdb1 ext4 label=FOO uuid=BAR

[lp:~wesley-wiedenmeier/curtin/mkfs]

350. By Wesley Wiedenmeier on 2016-01-07

Remove config parsing from curtin.commands.mkfs and add flags to pass in label
and uuid

351. By Wesley Wiedenmeier on 2016-01-08

Added uuid and label kwargs to mkfs.mkfs to simplify usage

352. By Wesley Wiedenmeier on 2016-01-11

Added force flag as kwarg in block.mkfs

353. By Wesley Wiedenmeier on 2016-01-11

Removed extra_flags parameter from block.mkfs and rewrote logic to append flags

354. By Wesley Wiedenmeier on 2016-01-11

Updated docstring for mkfs to remove explaination of extra_flags

355. By Wesley Wiedenmeier on 2016-01-11

Added strict flag to mkfs command to enforce filesystem label length limits and
flags not supported by some some filesystems

356. By Wesley Wiedenmeier on 2016-01-11

Move check for btrfs+precise into mkfs

357. By Wesley Wiedenmeier on 2016-01-12

Sorted command tables in block.mkfs

[Scott Moser (smoser)]

Hey.
One more thing on this. I just gave it a try.
$ rm -f /tmp/my.img; truncate --size 1G /tmp/my.img ; ./bin/curtin mkfs /tmp/my.img
invalid block dev path '/tmp/my.img'

I'd like that fixed, so I changed it to just verify that the path exists.

Then:
$ rm -f /tmp/my.img; truncate --size 1G /tmp/my.img ; ./bin/curtin mkfs /tmp/my.img
<hang>

I hit enter and I see:
Unexpected error while running command.
Command: ['mkfs.ext4', '/tmp/my.img']
Exit code: 1
Reason: -
Stdout: '/tmp/my.img is not a block special device.\nProceed anyway? (y,n) '
Stderr: 'mke2fs 1.42.9 (4-Feb-2014)\n'

It was expecting input from me but was hiding/capturing output.

We need to change the subp implementation to not use 'subprocess.pipe'.

Below is the patch for that. Then we get the same output as above, without the hang.. which is good.

=== modified file 'curtin/block/mkfs.py'
--- curtin/block/mkfs.py 2016-01-12 15:02:45 +0000
+++ curtin/block/mkfs.py 2016-01-15 21:49:20 +0000
@@ -22,6 +22,7 @@
 from curtin import block

 import string
+import os

 mkfs_commands = {
         "btrfs": "mkfs.btrfs",
@@ -119,8 +120,10 @@
        finds old data or filesystems on the partition.
        """

- if path is None or not block.is_valid_device(path):
+ if path is None:
         raise ValueError("invalid block dev path '%s'" % path)
+ if not os.path.exists(path):
+ raise ValueError("'%s': no such file or directory" % path)

     fs_family = specific_to_family.get(fstype, fstype)
     mkfs_cmd = mkfs_commands.get(fstype)

=== modified file 'curtin/util.py'
--- curtin/util.py 2015-12-18 19:47:48 +0000
+++ curtin/util.py 2016-01-15 21:51:07 +0000
@@ -39,6 +39,7 @@
     if rcs is None:
         rcs = [0]

+ devnull_fp = None
     try:
         if not logstring:
             LOG.debug(("Running command %s with allowed return codes %s"
@@ -52,7 +53,10 @@
         if capture:
             stdout = subprocess.PIPE
             stderr = subprocess.PIPE
- if data is not None:
+ if data is None:
+ devnull_fp = open(os.devnull)
+ stdin = devnull_fp
+ else:
             stdin = subprocess.PIPE
         sp = subprocess.Popen(args, stdout=stdout,
                               stderr=stderr, stdin=stdin,
@@ -74,6 +78,10 @@
             err = ldecode(err)
     except OSError as e:
         raise ProcessExecutionError(cmd=args, reason=e)
+ finally:
+ if devnull_fp:
+ devnull_fp.close()
+
     rc = sp.returncode # pylint: disable=E1101
     if rc not in rcs:
         raise ProcessExecutionError(stdout=out, stderr=err,

[Scott Moser (smoser)]

So that fix in pastebin is http://paste.ubuntu.com/14511520/

The result then we can actually:
$ rm -f /tmp/my.img; truncate --size 1G /tmp/my.img ; ./bin/curtin mkfs --force /tmp/my.img
$ blkid /tmp/my.img
/tmp/my.img: UUID="3268e5cc-ac13-4086-8786-e45c0927385f" TYPE="ext4"

[lp:~wesley-wiedenmeier/curtin/mkfs]

358. By Wesley Wiedenmeier on 2016-01-15

Applied patch from http://paste.ubuntu.com/14511520/

 - will operate on non block devices such as disk images
 - will not hang silently if waiting for confirmation

359. By Wesley Wiedenmeier on 2016-01-15

Updated unit tests to mock os.path.exists instead of block.is_valid_device

360. By Wesley Wiedenmeier on 2016-01-15

block.mkfs returns uuid of created filesystem

 - if no uuid is passed into command uuid.uuid1() is used to generate one
 - if the mkfs command for the filesystem type does not support passing in uuid
   then blkid will be used to determine uuid after fs is created
 - if blkid fails no exceptions will be thrown as mkfs command did still
   succeed

361. By Wesley Wiedenmeier on 2016-01-15

Updated unit tests to verify that mkfs is generating a uuid and to avoid
potential errors from not mocking block

362. By Wesley Wiedenmeier on 2016-01-15

Print message after creating filesystem in commands.mkfs

363. By Wesley Wiedenmeier on 2016-01-15

Added block.mkfs.valid_fstypes() to list fstypes we know how to support

364. By Wesley Wiedenmeier on 2016-01-15

Use 'choices': valid_fstypes() in commands.mkfs arguments so that help message
lists valid fstypes

=== added file 'curtin/block/mkfs.py'
--- curtin/block/mkfs.py	1970-01-01 00:00:00 +0000
+++ curtin/block/mkfs.py	2016-01-15 23:10:42 +0000
@@ -0,0 +1,197 @@
+#   Copyright (C) 2016 Canonical Ltd.
+#
+#   Author: Wesley Wiedenmeier <[e-mail]>
+#
+#   Curtin is free software: you can redistribute it and/or modify it under
+#   the terms of the GNU Affero General Public License as published by the
+#   Free Software Foundation, either version 3 of the License, or (at your
+#   option) any later version.
+#
+#   Curtin is distributed in the hope that it will be useful, but WITHOUT ANY
+#   WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+#   FOR A PARTICULAR PURPOSE.  See the GNU Affero General Public License for
+#   more details.
+#
+#   You should have received a copy of the GNU Affero General Public License
+#   along with Curtin.  If not, see <http://www.gnu.org/licenses/>.
+
+# This module wraps calls to mkfs.<fstype> and determines the appropriate flags
+# for each filesystem type
+
+from curtin import util
+from curtin import block
+
+import string
+import os
+from uuid import uuid1
+
+mkfs_commands = {
+        "btrfs": "mkfs.btrfs",
+        "ext2": "mkfs.ext2",
+        "ext3": "mkfs.ext3",
+        "ext4": "mkfs.ext4",
+        "fat": "mkfs.fat",
+        "fat12": "mkfs.fat",
+        "fat16": "mkfs.fat",
+        "fat32": "mkfs.fat",
+        "jfs": "jfs_mkfs",
+        "ntfs": "mkntfs",
+        "reiserfs": "mkfs.reiserfs",
+        "swap": "mkswap",
+        "xfs": "mkfs.xfs"
+        }
+
+specific_to_family = {
+        "ext2": "ext",
+        "ext3": "ext",
+        "ext4": "ext",
+        "fat12": "fat",
+        "fat16": "fat",
+        "fat32": "fat",
+        }
+
+label_length_limits = {
+        "btrfs": 256,
+        "ext": 16,
+        "fat": 11,
+        "jfs": 16,  # see jfs_tune manpage
+        "ntfs": 32,
+        "reiserfs": 16,
+        "swap": 15,  # not in manpages, found experimentally
+        "xfs": 12
+        }
+
+family_flag_mappings = {
+        "label": {"btrfs": "--label",
+                  "ext": "-L",
+                  "fat": "-n",
+                  "jfs": "-L",
+                  "ntfs": "--label",
+                  "reiserfs": "--label",
+                  "swap": "--label",
+                  "xfs": "-L"},
+        "uuid": {"btrfs": "--uuid",
+                 "ext": "-U",
+                 "reiserfs": "--uuid",
+                 "swap": "--uuid"},
+        "force": {"btrfs": "--force",
+                  "ext": "-F",
+                  "ntfs": "--force",
+                  "reiserfs": "-f",
+                  "swap": "--force",
+                  "xfs": "-f"},
+        "fatsize": {"fat": "-F"},
+        "quiet": {"ext": "-q",
+                  "ntfs": "-q",
+                  "reiserfs": "-q",
+                  "xfs": "--quiet"}
+        }
+
+
+def valid_fstypes():
+    return list(mkfs_commands.keys())
+
+
+def get_flag_mapping(flag_name, fs_family, param=None, strict=False):
+    ret = []
+    flag_sym_families = family_flag_mappings.get(flag_name)
+    if flag_sym_families is None:
+        raise ValueError("unsupported flag '%s'" % flag_name)
+    flag_sym = flag_sym_families.get(fs_family)
+    if flag_sym is None:
+        if strict:
+            raise ValueError("flag '%s' not supported by fs family '%s'" %
+                             flag_name, fs_family)
+    else:
+        ret = [flag_sym]
+        if param is not None:
+            ret.append(param)
+    return ret
+
+
+def mkfs(path, fstype, strict=False, label=None, uuid=None, force=False):
+    """Make filesystem on block device with given path using given fstype and
+       appropriate flags for filesystem family.
+
+       Filesystem uuid and label can be passed in as kwargs. By default no
+       label or uuid will be used. If a filesystem label is too long curtin
+       will raise a ValueError if the strict flag is true or will truncate
+       it to the maximum possible length.
+
+       If a flag is not supported by a filesystem family mkfs will raise a
+       ValueError if the strict flag is true or silently ignore it otherwise.
+
+       Force can be specified to force the mkfs command to continue even if it
+       finds old data or filesystems on the partition.
+       """
+
+    if path is None:
+        raise ValueError("invalid block dev path '%s'" % path)
+    if not os.path.exists(path):
+        raise ValueError("'%s': no such file or directory" % path)
+
+    fs_family = specific_to_family.get(fstype, fstype)
+    mkfs_cmd = mkfs_commands.get(fstype)
+    if not mkfs_cmd:
+        raise ValueError("unsupported fs type '%s'" % fstype)
+
+    if util.which(mkfs_cmd) is None:
+        raise ValueError("need '%s' but it could not be found" % mkfs_cmd)
+
+    cmd = [mkfs_cmd]
+
+    if force:
+        # On precise mkfs.btrfs does not have a force flag, though it does in
+        # later releases
+        if not (util.lsb_release()['codename'] == "precise" and fstype ==
+                "btrfs"):
+            cmd.extend(get_flag_mapping("force", fs_family, strict=strict))
+    if label is not None:
+        limit = label_length_limits.get(fs_family)
+        if len(label) > limit:
+            if strict:
+                raise ValueError("length of fs label for '%s' exceeds max \
+                                 allowed for fstype '%s'. max is '%s'"
+                                 % (path, fstype, limit))
+            else:
+                label = label[:limit]
+        cmd.extend(get_flag_mapping("label", fs_family, param=label,
+                                    strict=strict))
+
+    # If uuid is not specified, generate one and try to use it
+    if uuid is None:
+        uuid = str(uuid1())
+    cmd.extend(get_flag_mapping("uuid", fs_family, param=uuid, strict=strict))
+
+    if fs_family == "fat":
+        fat_size = fstype.strip(string.ascii_letters)
+        if fat_size in ["12", "16", "32"]:
+            cmd.extend(get_flag_mapping("fatsize", fs_family, param=fat_size,
+                                        strict=strict))
+
+    cmd.append(path)
+    util.subp(cmd, capture=True)
+
+    # if fs_family does not support specifying uuid then use blkid to find it
+    # if blkid is unable to then just return None for uuid
+    if fs_family not in family_flag_mappings['uuid']:
+        try:
+            uuid = block.blkid()[path]['UUID']
+        except:
+            pass
+
+    # return uuid, may be none if it could not be specified and blkid could not
+    # find it
+    return uuid
+
+
+def mkfs_from_config(path, info, strict=False):
+    """Make filesystem on block device with given path according to storage
+       config given"""
+    fstype = info.get('fstype')
+    if fstype is None:
+        raise ValueError("fstype must be specified")
+    # NOTE: Since old metadata on partitions that have not been wiped can cause
+    #       some mkfs commands to refuse to work, it's best to use force=True
+    mkfs(path, fstype, strict=strict, force=True, uuid=info.get('uuid'),
+         label=info.get('label'))

=== modified file 'curtin/commands/block_meta.py'
--- curtin/commands/block_meta.py	2016-01-07 18:53:54 +0000
+++ curtin/commands/block_meta.py	2016-01-15 23:10:42 +0000
@@ -19,6 +19,7 @@
 from curtin import (block, config, util)
 from curtin.block import mdadm
 from curtin.log import LOG
+from curtin.block import mkfs
 
 from . import populate_one_subcmd
 from curtin.udev import compose_udev_equality
@@ -26,7 +27,6 @@
 import glob
 import os
 import platform
-import string
 import sys
 import tempfile
 import time
@@ -635,10 +635,7 @@
 
 
 def format_handler(info, storage_config):
-    fstype = info.get('fstype')
     volume = info.get('volume')
-    part_label = info.get('label')
-    uuid = info.get('uuid')
     if not volume:
         raise ValueError("volume must be specified for partition '%s'" %
                          info.get('id'))
@@ -651,51 +648,8 @@
         # Volume marked to be preserved, not formatting
         return
 
-    # Generate mkfs command and run
-    if fstype in ["ext4", "ext3"]:
-        cmd = ['mkfs.%s' % fstype, '-F', '-q']
-        if part_label:
-            if len(part_label) > 16:
-                raise ValueError(
-                    "ext3/4 partition labels cannot be longer than "
-                    "16 characters")
-            else:
-                cmd.extend(["-L", part_label])
-        if uuid:
-            cmd.extend(["-U", uuid])
-        cmd.append(volume_path)
-    elif fstype in ["btrfs"]:
-        cmd = ['mkfs.%s' % fstype]
-        if util.lsb_release()['codename'] != "precise":
-            cmd.extend(['-f'])
-        if part_label:
-                cmd.extend(["-L", part_label])
-        if uuid:
-            cmd.extend(["-U", uuid])
-        cmd.append(volume_path)
-    elif fstype in ["fat12", "fat16", "fat32", "fat"]:
-        cmd = ["mkfs.fat"]
-        fat_size = fstype.strip(string.ascii_letters)
-        if fat_size in ["12", "16", "32"]:
-            cmd.extend(["-F", fat_size])
-        if part_label:
-            if len(part_label) > 11:
-                raise ValueError(
-                    "fat partition names cannot be longer than "
-                    "11 characters")
-            cmd.extend(["-n", part_label])
-        cmd.append(volume_path)
-    elif fstype == "swap":
-        cmd = ["mkswap", volume_path]
-    else:
-        # See if mkfs.<fstype> exists. If so try to run it.
-        try:
-            util.subp(["which", "mkfs.%s" % fstype])
-            cmd = ["mkfs.%s" % fstype, volume_path]
-        except util.ProcessExecutionError:
-            raise ValueError("fstype '%s' not supported" % fstype)
-    LOG.info("formatting volume '%s' with format '%s'" % (volume_path, fstype))
-    logtime(' '.join(cmd), util.subp, cmd)
+    # Make filesystem using block library
+    mkfs.mkfs_from_config(volume_path, info)
 
 
 def mount_handler(info, storage_config):

=== modified file 'curtin/commands/mkfs.py'
--- curtin/commands/mkfs.py	2015-11-18 17:15:08 +0000
+++ curtin/commands/mkfs.py	2016-01-15 23:10:42 +0000
@@ -15,18 +15,11 @@
 #   You should have received a copy of the GNU Affero General Public License
 #   along with Curtin.  If not, see <http://www.gnu.org/licenses/>.
 
-import curtin.config
-from curtin.log import LOG
-from curtin import config
-from curtin import util
 from . import populate_one_subcmd
-from curtin.commands.block_meta import get_path_to_storage_volume
-
-from collections import OrderedDict
-
-import os
+from curtin.block.mkfs import mkfs as run_mkfs
+from curtin.block.mkfs import valid_fstypes
+
 import sys
-import string
 
 CMD_ARGUMENTS = (
     (('devices',
@@ -35,84 +28,30 @@
        'metavar': 'DEVICE', 'action': 'store', 'nargs': '+'}),
      (('-f', '--fstype'),
       {'help': 'filesystem type to use. default is ext4',
+       'choices': valid_fstypes(),
        'default': 'ext4', 'action': 'store'}),
-     (('-c', '--config'),
-      {'help': 'read configuration from cfg', 'action': 'append',
-       'metavar': 'CONFIG', 'dest': 'cfgopts', 'default': []})
+     (('-l', '--label'),
+      {'help': 'label to use for filesystem', 'action': 'store'}),
+     (('-u', '--uuid'),
+      {'help': 'uuid to use for filesystem', 'action': 'store'}),
+     (('-s', '--strict'),
+      {'help': 'exit if mkfs cannot do exactly what is specified',
+       'action': 'store_true', 'default': False}),
+     (('-F', '--force'),
+      {'help': 'continue if some data already exists on device',
+       'action': 'store_true', 'default': False})
      )
 )
 
 
-def format_blockdev(volume_path, fstype, part_id=None):
-    if not part_id:
-        part_id = volume_path.split("/")[-1]
-
-    # Generate mkfs command and run
-    if fstype in ["ext4", "ext3"]:
-        if len(part_id) > 16:
-            raise ValueError("ext3/4 partition labels cannot be longer than \
-                16 characters")
-        cmd = ['mkfs.%s' % fstype, '-F', '-q', '-L', part_id, volume_path]
-    elif fstype in ["fat12", "fat16", "fat32", "fat"]:
-        cmd = ["mkfs.fat"]
-        fat_size = fstype.strip(string.ascii_letters)
-        if fat_size in ["12", "16", "32"]:
-            cmd.extend(["-F", fat_size])
-        if len(part_id) > 11:
-            raise ValueError("fat partition names cannot be longer than \
-                11 characters")
-        cmd.extend(["-n", part_id, volume_path])
-    else:
-        # See if mkfs.<fstype> exists. If so try to run it.
-        try:
-            util.subp(["which", "mkfs.%s" % fstype])
-            cmd = ["mkfs.%s" % fstype, volume_path]
-        except util.ProcessExecutionError:
-            raise ValueError("fstype '%s' not supported" % fstype)
-    LOG.info("formatting volume '%s' with format '%s'" % (volume_path, fstype))
-    util.subp(cmd)
-
-
-def format_storage_item(info, storage_config):
-    fstype = info.get('fstype')
-    volume = info.get('volume')
-    part_id = info.get('id')
-    if not volume:
-        raise ValueError("volume must be specified for partition '%s'" %
-                         info.get('id'))
-
-    # Get path to volume
-    volume_path = get_path_to_storage_volume(volume, storage_config)
-
-    # Call format_blockdev
-    format_blockdev(volume_path, fstype, part_id=part_id)
-
-
 def mkfs(args):
-    state = util.load_command_environment()
-    cfg = config.load_command_config(args, state)
-
-    if args.cfgopts:
-        for filename in args.cfgopts:
-            curtin.config.merge_config_fp(cfg, open(filename))
-
-    if "storage" in cfg:
-        storage_config = OrderedDict((d["id"], d) for (i, d) in
-                                     enumerate(cfg.get("storage")))
-    else:
-        storage_config = {}
-
     for device in args.devices:
-        if device in storage_config:
-            # Device is in storage config
-            format_storage_item(storage_config.get(device), storage_config)
-        elif "/dev/" in device and os.path.exists(device):
-            # Device is path to block dev
-            format_blockdev(device, args.fstype)
-        else:
-            # Bad argument
-            raise ValueError("device '%s' is neither an item in storage "
-                             "config nor a block device" % device)
+        uuid = run_mkfs(device, args.fstype, strict=args.strict,
+                        uuid=args.uuid, label=args.label,
+                        force=args.force)
+
+        print("Created '%s' filesystem in '%s' with uuid '%s' and label '%s'" %
+              (args.fstype, device, uuid, args.label))
 
     sys.exit(0)
 

=== modified file 'curtin/util.py'
--- curtin/util.py	2015-12-18 19:47:48 +0000
+++ curtin/util.py	2016-01-15 23:10:42 +0000
@@ -39,6 +39,7 @@
     if rcs is None:
         rcs = [0]
 
+    devnull_fp = None
     try:
         if not logstring:
             LOG.debug(("Running command %s with allowed return codes %s"
@@ -52,7 +53,10 @@
         if capture:
             stdout = subprocess.PIPE
             stderr = subprocess.PIPE
-        if data is not None:
+        if data is None:
+            devnull_fp = open(os.devnull)
+            stdin = devnull_fp
+        else:
             stdin = subprocess.PIPE
         sp = subprocess.Popen(args, stdout=stdout,
                               stderr=stderr, stdin=stdin,
@@ -74,6 +78,10 @@
             err = ldecode(err)
     except OSError as e:
         raise ProcessExecutionError(cmd=args, reason=e)
+    finally:
+        if devnull_fp:
+            devnull_fp.close()
+
     rc = sp.returncode  # pylint: disable=E1101
     if rc not in rcs:
         raise ProcessExecutionError(stdout=out, stderr=err,

=== added file 'tests/unittests/test_block_mkfs.py'
--- tests/unittests/test_block_mkfs.py	1970-01-01 00:00:00 +0000
+++ tests/unittests/test_block_mkfs.py	2016-01-15 23:10:42 +0000
@@ -0,0 +1,134 @@
+from curtin.block import mkfs
+
+from unittest import TestCase
+import mock
+
+
+class TestBlockMkfs(TestCase):
+    test_uuid = "fb26cc6c-ae73-11e5-9e38-2fb63f0c3155"
+
+    def _get_config(self, fstype):
+        return {"fstype": fstype, "type": "format", "id": "testfmt",
+                "volume": "null", "label": "format1",
+                "uuid": self.test_uuid}
+
+    def _assert_same_flags(self, call, expected):
+        for flag in expected:
+            if type(flag) == list:
+                flag_name = flag[0]
+                flag_val = flag[1]
+                self.assertIn(flag_name, call)
+                flag_index = call.index(flag_name)
+                self.assertTrue(len(call) > flag_index)
+                self.assertEquals(call[flag_index + 1], flag_val)
+                call.remove(flag_name)
+                call.remove(flag_val)
+            else:
+                self.assertIn(flag, call)
+                call.remove(flag)
+        # Only remaining vals in call should be mkfs.fstype and dev path
+        self.assertEquals(len(call), 2)
+
+    @mock.patch("curtin.block.mkfs.block")
+    @mock.patch("curtin.block.mkfs.os")
+    @mock.patch("curtin.block.mkfs.util")
+    def _run_mkfs_with_config(self, config, expected_cmd, expected_flags,
+                              mock_util, mock_os, mock_block,
+                              release="wily", strict=False):
+        # Pretend we are on wily as there are no known edge cases for it
+        mock_util.lsb_release.return_value = {"codename": release}
+        mock_os.path.exists.return_value = True
+
+        mkfs.mkfs_from_config("/dev/null", config, strict=strict)
+        self.assertTrue(mock_util.subp.called)
+        calls = mock_util.subp.call_args_list
+        self.assertEquals(len(calls), 1)
+
+        # Get first function call, tuple of first positional arg and its
+        # (nonexistant) keyword arg, and unpack to get cmd
+        call = calls[0][0][0]
+        self.assertEquals(call[0], expected_cmd)
+        self._assert_same_flags(call, expected_flags)
+
+    def test_mkfs_ext(self):
+        conf = self._get_config("ext4")
+        expected_flags = [["-L", "format1"], "-F",
+                          ["-U", self.test_uuid]]
+        self._run_mkfs_with_config(conf, "mkfs.ext4", expected_flags)
+
+    def test_mkfs_btrfs(self):
+        conf = self._get_config("btrfs")
+        expected_flags = [["--label", "format1"], "--force",
+                          ["--uuid", self.test_uuid]]
+        self._run_mkfs_with_config(conf, "mkfs.btrfs", expected_flags)
+
+        # Test precise+btrfs edge case, force should not be used
+        expected_flags.remove("--force")
+        self._run_mkfs_with_config(conf, "mkfs.btrfs", expected_flags,
+                                   release="precise")
+
+    def test_mkfs_fat(self):
+        conf = self._get_config("fat32")
+        expected_flags = [["-n", "format1"], ["-F", "32"]]
+        self._run_mkfs_with_config(conf, "mkfs.fat", expected_flags)
+
+    def test_mkfs_invalid_fstype(self):
+        """Do not proceed if fstype is None or invalid"""
+        with self.assertRaises(ValueError):
+            conf = self._get_config(None)
+            self._run_mkfs_with_config(conf, "mkfs.ext4", [])
+        with self.assertRaises(ValueError):
+            conf = self._get_config("fakefilesystemtype")
+            self._run_mkfs_with_config(conf, "mkfs.ext3", [])
+
+    def test_mkfs_invalid_label(self):
+        """Do not proceed if filesystem label is too long"""
+        with self.assertRaises(ValueError):
+            conf = self._get_config("ext4")
+            conf['label'] = "thislabelislongerthan16chars"
+            self._run_mkfs_with_config(conf, "mkfs.ext4", [], strict=True)
+
+        conf = self._get_config("swap")
+        expected_flags = ["--force", ["--label", "abcdefghijklmno"],
+                          ["--uuid", conf['uuid']]]
+        conf['label'] = "abcdefghijklmnop"  # 16 chars, 15 is max
+
+        # Raise error, do not truncate with strict = True
+        with self.assertRaises(ValueError):
+            self._run_mkfs_with_config(conf, "mkswap", expected_flags,
+                                       strict=True)
+
+        # Do not raise with strict = False
+        self._run_mkfs_with_config(conf, "mkswap", expected_flags)
+
+    @mock.patch("curtin.block.mkfs.block")
+    @mock.patch("curtin.block.mkfs.util")
+    @mock.patch("curtin.block.mkfs.os")
+    def test_mkfs_kwargs(self, mock_os, mock_util, mock_block):
+        """Ensure that kwargs are being followed"""
+        mkfs.mkfs("/dev/null", "ext4", [], uuid=self.test_uuid,
+                  label="testlabel", force=True)
+        expected_flags = ["-F", ["-L", "testlabel"], ["-U", self.test_uuid]]
+        calls = mock_util.subp.call_args_list
+        self.assertEquals(len(calls), 1)
+        call = calls[0][0][0]
+        self.assertEquals(call[0], "mkfs.ext4")
+        self._assert_same_flags(call, expected_flags)
+
+    @mock.patch("curtin.block.mkfs.os")
+    def test_mkfs_invalid_block_device(self, mock_os):
+        """Do not proceed if block device is none or is not valid block dev"""
+        with self.assertRaises(ValueError):
+            mock_os.path.exists.return_value = False
+            mkfs.mkfs("/dev/null", "ext4")
+        with self.assertRaises(ValueError):
+            mock_os.path.exists.return_value = True
+            mkfs.mkfs(None, "ext4")
+
+    @mock.patch("curtin.block.mkfs.util")
+    @mock.patch("curtin.block.mkfs.os")
+    def test_mkfs_generates_uuid(self, mock_os, mock_util):
+        """Ensure that block.mkfs generates and returns a uuid if None is
+           provided"""
+        uuid = mkfs.mkfs("/dev/null", "ext4")
+        self.assertIsNotNone(uuid)