Merge lp:~salgado/linaro-image-tools/partition-setup into lp:linaro-image-tools/11.11

[Guilherme Salgado]

This is a rather large branch with lots of changes that were needed to port
some shell code that was interrelated and thus needed to be ported at once.

- Change cmd_runner.run() to return a Popen instance so that it's more
  flexible

- Simplify create_partitions now that it's only called from python code

- Create a new partitions.py module which provides a single function for doing
  everything related with partitioning. That's where most of the new code is,
  but everything should be self-explanatory (or have comments) there.

- Merge partition_size.py into the above file

- Extend MockCmdRunnerPopen so that it stores the arguments passed in to each
  of its invocations; useful when testing functions that run multiple
  subprocesses.

- Create a new tests/integration.txt file which just lists some integration
  tests that are useful but must not be automated.

[Martin Ohlsson (martin-ohlson)]

> === modified file 'media_create/create_partitions.py'
> --- media_create/create_partitions.py 2010-12-09 19:58:24 +0000
> +++ media_create/create_partitions.py 2010-12-13 14:51:10 +0000
>
> cmd_runner.run(
> - ['parted', '-s', device, 'mklabel', 'msdos'], as_root=True)
> + ['parted', '-s', media.path, 'mklabel', 'msdos'], as_root=True)
> + # It sems to be necessary to sleep a bit here to avoid a race
> + # condition with the sfdisk commands executed below. Try removing it
> + # and running the integration tests to see how it fails.
> + time.sleep(0.5)

Isn't it possible to wait() for the parted command to finish?

/Martin

[Peter Maydell (pmaydell)]

146 - # Round the size of the raw disk image up to a multiple of 256K
147 - # so it is an exact number of SD card erase blocks in length.
148 - # Otherwise Linux under qemu cannot access the last part of the
149 - # card and is likely to complain that the last partition on the
150 - # disk has been truncated.
151 - IMAGE_SIZE=$(((($IMAGE_SIZE-1)/(1024*256)+1)*(1024*256)))

So this bug fix has gone from the shell script but I can't find the bit in the Python which implements it. Am I missing something?

[Guilherme Salgado (salgado)]

On Mon, 2010-12-13 at 16:25 +0000, Martin Ohlsson wrote:
> > === modified file 'media_create/create_partitions.py'
> > --- media_create/create_partitions.py 2010-12-09 19:58:24 +0000
> > +++ media_create/create_partitions.py 2010-12-13 14:51:10 +0000
> >
> > cmd_runner.run(
> > - ['parted', '-s', device, 'mklabel', 'msdos'], as_root=True)
> > + ['parted', '-s', media.path, 'mklabel', 'msdos'], as_root=True)
> > + # It sems to be necessary to sleep a bit here to avoid a race
> > + # condition with the sfdisk commands executed below. Try removing it
> > + # and running the integration tests to see how it fails.
> > + time.sleep(0.5)
>
> Isn't it possible to wait() for the parted command to finish?

Very good catch! Friday I updated a bunch of callsites that were
expecting cmd_runner.run() to wait() for them but forgot this one and
today it didn't occur to me that the race could be a consequence of the
now-missing wait() call. I've updated this and a few other callsites
that I missed to update when I changed cmd_runner.run(). Thanks!

[lp:~salgado/linaro-image-tools/partition-setup]

193. By Guilherme Salgado on 2010-12-13

Update a few cmd_runner.run() callsites that were not wait()ing

[Guilherme Salgado (salgado)]

On Mon, 2010-12-13 at 16:39 +0000, Peter Maydell wrote:
> 146 - # Round the size of the raw disk image up to a multiple of 256K
> 147 - # so it is an exact number of SD card erase blocks in length.
> 148 - # Otherwise Linux under qemu cannot access the last part of the
> 149 - # card and is likely to complain that the last partition on the
> 150 - # disk has been truncated.
> 151 - IMAGE_SIZE=$(((($IMAGE_SIZE-1)/(1024*256)+1)*(1024*256)))
>
> So this bug fix has gone from the shell script but I can't find the bit in the Python which implements it. Am I missing something

Looks like it was lost in translation indeed. However, qemu was able to
boot an image I generated using the code on this branch. Would the lack
of this fix render the image unusable or should I expect to see other
symptoms? Is there any chance the fix is no longer needed or is needed
only for certain versions of qemu?

[Peter Maydell (pmaydell)]

The symptom would be that on bootup fsck complains about the filesystem being larger than the partition. It may well happen only for certain image sizes -- if you happened to get l-m-c to use an image size that's already a multiple of 256K then you wouldn't see any effect.

(We've also had problems reported even with the bugfix, eg https://bugs.launchpad.net/linux-linaro/+bug/673335 -- so something's still not quite right.)

However, I think it would be better to deal with possible improvements to that bug fix elsewhere, not as part of this conversion-to-python process.

[James Westby (james-w)]

Hi,

I haven't read this in the detail that I would like, but I couldn't see any issues.

Could I suggest that your integration tests tell you what the expected result should
be?

Thanks,

James

[Martin Ohlsson (martin-ohlson)]

Hi Guilherme,

I read the code a couple of times without noticing anything strange besides my previous comment.

/Martin

review: Approve

[Guilherme Salgado (salgado)]

On Mon, 2010-12-13 at 17:37 +0000, Peter Maydell wrote:
> The symptom would be that on bootup fsck complains about the
> filesystem being larger than the partition. It may well happen only
> for certain image sizes -- if you happened to get l-m-c to use an
> image size that's already a multiple of 256K then you wouldn't see any
> effect.
>
> (We've also had problems reported even with the bugfix, eg
> https://bugs.launchpad.net/linux-linaro/+bug/673335 -- so something's
> still not quite right.)
>
> However, I think it would be better to deal with possible improvements
> to that bug fix elsewhere, not as part of this conversion-to-python
> process.

Agreed, but if I knew how to reproduce I could add that as a comment in
the code to save time when somebody gets to work on those specific bits.

The bug-fix in now present in the python version.

[lp:~salgado/linaro-image-tools/partition-setup]

194. By Guilherme Salgado on 2010-12-14

A few tweaks a bug fixes suggested by reviewers

=== modified file 'README'
--- README	2010-12-03 13:22:03 +0000
+++ README	2010-12-14 13:31:33 +0000
@@ -11,6 +11,8 @@
   - python-debian >= 0.1.16ubuntu1
   - python-argparse
   - dpkg-dev
+  - python-parted
+  - python-dbus
 
 And run the following command:
 

=== modified file 'linaro-media-create'
--- linaro-media-create	2010-12-12 20:48:29 +0000
+++ linaro-media-create	2010-12-14 13:31:33 +0000
@@ -34,7 +34,7 @@
 MMC_PART_OFFSET=0
 BOOTFS_STEP="create_boot_cmd populate_boot"
 ROOTFS_STEP="populate_rootfs"
-PARTITION_STEP="create_partitions"
+SHOULD_CREATE_PARTITIONS="yes"
 mmc_option="0:1"
 boot_args_options="rootwait ro"
 HWPACK_FORCE_YES="no"
@@ -272,13 +272,13 @@
       BOOTFS_STEP=""
       ;;
     --no-part)
-      PARTITION_STEP=""
+      SHOULD_CREATE_PARTITIONS="no"
       ;;
   esac
   shift
 done
 
-DEPLOY_STEPS="$PARTITION_STEP prepare_partitions $BOOTFS_STEP $ROOTFS_STEP"
+DEPLOY_STEPS="setup_partitions $BOOTFS_STEP $ROOTFS_STEP"
 
 python -m media_create.ensure_command mkimage uboot-mkimage
 python -m media_create.ensure_command uuidgen uuid-runtime
@@ -309,33 +309,6 @@
 
 RFS_UUID=`uuidgen -r`
 
-get_device_by_id() {
-  if [ ! "${IMAGE_FILE}" ]; then
-    for device in /dev/disk/by-id/*; do
-      if [ `realpath $device` = $DEVICE ]; then
-        case "$device" in
-          *-part[0-9]*)
-            echo "device $DEVICE must not be a partition part ($device)" >&2
-            exit 1
-          ;;
-        esac
-        for part_id in `ls "$device-part"*`; do
-          part=`realpath $part_id`
-          part_no=`echo $part_id | sed -e 's/.*-part//g'`
-          # echo "part $part_no found: $part_id" 1>&2
-          # if there is a non-fs-data part (e.g. mx51evk), MMC_PART_OFFSET=1
-          if test "$part_no" = $((1 + $MMC_PART_OFFSET)); then
-            BOOTFS=$part
-          elif test "$part_no" = $((2 + $MMC_PART_OFFSET)) ; then
-            ROOTFS=$part
-          fi
-        done
-        break
-      fi
-    done
-  fi
-}
-
 install_hwpack() {
   chroot=${DIR}/binary
   # Make sure we unmount /proc in the chroot or else it can't be moved to the
@@ -445,33 +418,22 @@
   loop_devices="$loop_devices $1"
 }
 
-create_partitions() {
-  python -m media_create.create_partitions "${DEVIMAGE}" \
-      "${DEVICE-$IMAGE_FILE}" "$FAT_SIZE" "$HEADS" "$SECTORS" "$CYLINDER_ARG"
+setup_partitions() {
+  boot_and_root_devices=$(python -m media_create.partitions \
+      "$DEVIMAGE" "${DEVICE-$IMAGE_FILE}" "$FAT_SIZE" "$IMAGE_SIZE" \
+      "$SHOULD_CREATE_PARTITIONS")
+  eval $boot_and_root_devices
 
   if [ "${IMAGE_FILE}" ]; then
-    sizes_and_offsets=$(python -m media_create.partition_size "${IMAGE_FILE}")
-    eval $sizes_and_offsets
-    BOOTFS=$(sudo losetup -f --show "$IMAGE_FILE" --offset $VFATOFFSET \
-             --sizelimit $VFATSIZE)
     register_loopback "$BOOTFS"
-    ROOTFS=$(sudo losetup -f --show "$IMAGE_FILE" --offset $ROOTOFFSET \
-             --sizelimit $ROOTSIZE)
     register_loopback "$ROOTFS"
   fi
+
+  prepare_partitions
 }
 
-
 prepare_partitions() {
 
-  # sync and sleep a bit as the partitions may have been created in the
-  # previous step.
-  echo -n "waiting for partitioning to settle ..."
-  sync
-  sleep 3
-  echo "done."
-  get_device_by_id
-
   if [ "$BOOTFS_STEP" ]; then
     echo ""
     echo "Formating Boot Partition"
@@ -565,6 +527,7 @@
   echo "Creating /etc/flash-kernel.conf"
   echo ""
 
+  TARGET_BOOT_DEV=/dev/mmcblk0p$(( 1 + $MMC_PART_OFFSET ))
   echo "UBOOT_PART=${TARGET_BOOT_DEV}" | sudo tee "${ROOT_DISK}/etc/flash-kernel.conf" >/dev/null
 
   sync
@@ -572,39 +535,6 @@
   sudo umount "${ROOT_DISK}" || true
 }
 
-calculatesize() {
-  IMAGE_SIZE=${IMAGE_SIZE/G/M*1024}
-  IMAGE_SIZE=${IMAGE_SIZE/M/K*1024}
-  IMAGE_SIZE=${IMAGE_SIZE/K/*1024}
-  IMAGE_SIZE=$(($IMAGE_SIZE))
-}
-
-setup_sizes() {
-  HEADS=255
-  SECTORS=63
-  SECTORSIZE=512
-  if [ "${IMAGE_FILE}" ]; then
-    calculatesize
-    CYLINDERSIZE=$(($HEADS*$SECTORS*$SECTORSIZE))
-    CYLINDERS=$(($IMAGE_SIZE/$CYLINDERSIZE))
-    CYLINDER_ARG="-C $CYLINDERS"
-    IMAGE_SIZE=$(($CYLINDERS*$CYLINDERSIZE))
-    # Round the size of the raw disk image up to a multiple of 256K
-    # so it is an exact number of SD card erase blocks in length.
-    # Otherwise Linux under qemu cannot access the last part of the
-    # card and is likely to complain that the last partition on the
-    # disk has been truncated.
-    IMAGE_SIZE=$(((($IMAGE_SIZE-1)/(1024*256)+1)*(1024*256)))
-  else
-    CYLINDER_ARG=""
-  fi
-}
-
-# XXX: Apparently there's no need to run qemu-img as root.
-setup_image() {
-  sudo qemu-img create -f raw "$IMAGE_FILE" $IMAGE_SIZE
-}
-
 remove_image_file() {
   if [ -f "$IMAGE_FILE" ]; then
     rm -v $IMAGE_FILE
@@ -659,11 +589,6 @@
 trap cleanup_handler EXIT
 trap signal_handler INT TERM
 
-setup_sizes
-if [ "${IMAGE_FILE}" ]; then
-  setup_image
-fi
-
 serial_opts=""
 if [ "$consoles" ]; then
   for c in ${consoles}; do
@@ -767,9 +692,6 @@
   fi
 fi
 
-TARGET_BOOT_DEV=/dev/mmcblk0p$(( 1 + $MMC_PART_OFFSET ))
-TARGET_ROOT_DEV=/dev/mmcblk0p$(( 2 + $MMC_PART_OFFSET ))
-
 if [ ! "${DEVICE}" -a ! "${IMAGE_FILE}" ]; then
   usage
 fi

=== modified file 'media_create/cmd_runner.py'
--- media_create/cmd_runner.py	2010-12-09 19:17:14 +0000
+++ media_create/cmd_runner.py	2010-12-14 13:31:33 +0000
@@ -1,13 +1,19 @@
 import subprocess
 
 
-def run(args, as_root=False, stdout=None):
+def run(args, as_root=False, stdin=None, stdout=None, stderr=None):
     """Run the given command as a sub process.
 
+    Return a Popen instance.
+
+    Callsites must wait() or communicate() with the returned Popen instance.
+
     :param command: A list or tuple containing the command to run and the
                     arguments that should be passed to it.
     :param as_root: Should the given command be run as root (with sudo)?
-    :param stdout: Same as subprocess.Popen().
+    :param stdin: Same as in subprocess.Popen().
+    :param stdout: Same as in subprocess.Popen().
+    :param stderr: Same as in subprocess.Popen().
     """
     assert isinstance(args, (list, tuple)), (
         "The command to run must be a list or tuple, found: %s" % type(args))
@@ -16,9 +22,7 @@
     if as_root:
         args = args[:]
         args.insert(0, 'sudo')
-    proc = Popen(args, stdout=stdout)
-    proc.wait()
-    return proc.returncode
+    return Popen(args, stdin=stdin, stdout=stdout, stderr=stderr)
 
 
 class Popen(subprocess.Popen):

=== modified file 'media_create/create_partitions.py'
--- media_create/create_partitions.py	2010-12-09 19:58:24 +0000
+++ media_create/create_partitions.py	2010-12-14 13:31:33 +0000
@@ -1,11 +1,12 @@
 import subprocess
-import sys
+import time
 
 from media_create import cmd_runner
 
 
-def run_sfdisk_commands(commands, heads, sectors, cylinders_arg, device,
-                        as_root=True):
+# TODO: Merge this file with setup_partitions.py.
+def run_sfdisk_commands(commands, heads, sectors, cylinders, device,
+                        as_root=True, stderr=None):
     """Run the given commands under sfdisk.
 
     :param commands: A string of sfdisk commands; each on a separate line.
@@ -15,41 +16,33 @@
             '-D',
             '-H', str(heads),
             '-S', str(sectors)]
-    if cylinders_arg:
-        args.append(cylinders_arg)
+    if cylinders is not None:
+        args.extend(['-C', str(cylinders)])
     args.append(device)
-    # XXX: There's some stuff duplicated here from cmd_runner.run() but I
-    # don't see an easy way to consolidate them as a single function, so I'll
-    # leave it for later.
-    if as_root:
-        args = args[:]
-        args.insert(0, 'sudo')
-    proc = cmd_runner.Popen(
-        args, stdin=subprocess.PIPE, stdout=subprocess.PIPE,
-        stderr=subprocess.PIPE)
+    proc = cmd_runner.run(
+        args, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=stderr,
+        as_root=as_root)
     return proc.communicate("%s\n" % commands)
 
 
-def create_partitions(board, device, fat_size, heads, sectors, cylinders_arg):
-    """Partition the given device according to the board requirements.
+def create_partitions(board, media, fat_size, heads, sectors, cylinders=None):
+    """Partition the given media according to the board requirements.
 
     :param board: A string with the board type (e.g. beagle, panda, etc)
-    :param device: A string containing the path to the device to partition.
+    :param media: A setup_partitions.Media object to partition.
     :param fat_size: The type of FATs used in the boot partition (16 or 32).
     :param heads: Number of heads to use in the disk geometry of
         partitions.
     :param sectors: Number of sectors to use in the disk geometry of
         partitions.
-    :param cylinders_arg: A string of the form "-C NN" containing the number
-        of cylinders to use in the disk geometry of partitions.
+    :param cylinders: The number of cylinders to pass to sfdisk's -C argument.
+        If None the -C argument is not passed.
     """
-    stdout = []
-    stderr = []
-    is_block_device = device.startswith('/dev/')
-    if is_block_device:
+    if media.is_block_device:
         # Overwrite any existing partition tables with a fresh one.
-        cmd_runner.run(
-            ['parted', '-s', device, 'mklabel', 'msdos'], as_root=True)
+        proc = cmd_runner.run(
+            ['parted', '-s', media.path, 'mklabel', 'msdos'], as_root=True)
+        proc.wait()
 
     if fat_size == 32:
         partition_type = '0x0C'
@@ -60,28 +53,16 @@
         # Create a one cylinder partition for fixed-offset bootloader data at
         # the beginning of the image (size is one cylinder, so 8224768 bytes
         # with the first sector for MBR).
-        out, err = run_sfdisk_commands(
-            ',1,0xDA', heads, sectors, cylinders_arg, device)
-        stdout.append(out)
-        stderr.append(err)
+        run_sfdisk_commands(',1,0xDA', heads, sectors, cylinders, media.path)
 
     # Create a VFAT or FAT16 partition of 9 cylinders (74027520 bytes, ~70
     # MiB), followed by a Linux-type partition containing the rest of the free
     # space.
     sfdisk_cmd = ',9,%s,*\n,,,-' % partition_type
-    out, err = run_sfdisk_commands(
-        sfdisk_cmd, heads, sectors, cylinders_arg, device)
-    stdout.append(out)
-    stderr.append(err)
-    return "\n".join(stdout), "\n".join(stderr)
-
-
-if __name__ == "__main__":
-    board, device, fat_size, heads, sectors, cylinders_arg = sys.argv[1:]
-    fat_size = int(fat_size)
-    heads = int(heads)
-    sectors = int(sectors)
-    stdout, stderr = create_partitions(
-        board, device, fat_size, heads, sectors, cylinders_arg)
-    print stdout
-    print stderr
+    run_sfdisk_commands(sfdisk_cmd, heads, sectors, cylinders, media.path)
+
+    # Sync and sleep to wait for the partition to settle.
+    cmd_runner.run(['sync']).wait()
+    # Sleeping just 1 second seems to be enough here, but if we start getting
+    # errors because the disk is not partitioned then we should revisit this.
+    time.sleep(1)

=== removed file 'media_create/partition_size.py'
--- media_create/partition_size.py	2010-12-08 19:54:08 +0000
+++ media_create/partition_size.py	1970-01-01 00:00:00 +0000
@@ -1,42 +0,0 @@
-import sys
-
-from parted import (
-    Device,
-    Disk,
-    )
-
-
-def calculate_partition_size_and_offset(device):
-    """Return the size and offset of the boot and root partitions.
-
-    Both the size and offset are in sectors.
-
-    :param device: A string containing the path to the device.
-    :return: A 4-tuple containing the offset and size of the boot partition
-        followed by the offset and size of the root partition.
-    """
-    disk = Disk(Device(device))
-    vfat_partition = None
-    for partition in disk.partitions:
-        if 'boot' in partition.getFlagsAsString():
-            geometry = partition.geometry
-            vfat_offset = geometry.start * 512
-            vfat_size = geometry.length * 512
-            vfat_partition = partition
-
-    assert vfat_partition is not None, (
-        "Couldn't find boot partition on %s" % device)
-    linux_partition = vfat_partition.nextPartition()
-    geometry = linux_partition.geometry
-    linux_offset = geometry.start * 512
-    linux_size = geometry.length * 512
-    return vfat_size, vfat_offset, linux_size, linux_offset
-
-
-if __name__ == "__main__":
-    vsize, voffset, rsize, roffset = calculate_partition_size_and_offset(
-        sys.argv[1])
-    # These values need to be assigned to shell variables in our script, so we
-    # make its job easier by printing something it can pass on to eval.
-    print "VFATSIZE=%s VFATOFFSET=%s ROOTSIZE=%s ROOTOFFSET=%s" % (
-        vsize, voffset, rsize, roffset)

=== added file 'media_create/partitions.py'
--- media_create/partitions.py	1970-01-01 00:00:00 +0000
+++ media_create/partitions.py	2010-12-14 13:31:33 +0000
@@ -0,0 +1,193 @@
+import subprocess
+import sys
+
+import dbus
+from parted import (
+    Device,
+    Disk,
+    )
+
+from media_create import cmd_runner
+from media_create.create_partitions import create_partitions
+
+
+HEADS = 255
+SECTORS = 63
+SECTOR_SIZE = 512 # bytes
+CYLINDER_SIZE = HEADS * SECTORS * SECTOR_SIZE
+
+
+# I wonder if it'd make sense to convert this into a small shim which calls
+# the appropriate function for the given type of device?  I think it's still
+# small enough that there's not much benefit in doing that, but if it grows we
+# might want to do it.
+def setup_partitions(board, media, fat_size, image_size,
+                     should_create_partitions):
+    """Make sure the given device is partitioned to boot the given board.
+
+    :param board: The board's name, as a string.
+    :param media: The Media we should partition.
+    :param fat_size: The FAT size (16 or 32) for the boot partition.
+    :param image_size: The size of the image file, in case we're setting up a
+        QEMU image.
+    :param should_create_partitions: A string with values "yes" or "no",
+        specifying whether or not we should erase existing partitions and
+        create new ones. 
+    """
+    cylinders = None
+    if not media.is_block_device:
+        image_size_in_bytes = convert_size_to_bytes(image_size)
+        cylinders = image_size_in_bytes / CYLINDER_SIZE
+        image_size_in_bytes = cylinders * CYLINDER_SIZE
+        proc = cmd_runner.run(
+            ['qemu-img', 'create', '-f', 'raw', media.path, image_size],
+            stdout=open('/dev/null', 'w'))
+        proc.wait()
+
+    if should_create_partitions == "yes":
+        create_partitions(board, media, fat_size, HEADS, SECTORS, cylinders)
+
+    if media.is_block_device:
+        return get_boot_and_root_partitions_for_media(media)
+    else:
+        return get_boot_and_root_loopback_devices(media.path)
+
+
+def get_boot_and_root_loopback_devices(image_file):
+    """Return the boot and root loopback devices for the given image file.
+
+    Register the loopback devices as well.
+    """
+    vfat_size, vfat_offset, linux_size, linux_offset = (
+        calculate_partition_size_and_offset(image_file))
+    proc = cmd_runner.run(
+        ['losetup', '-f', '--show', image_file, '--offset',
+         str(vfat_offset), '--sizelimit', str(vfat_size)],
+        stdout=subprocess.PIPE, as_root=True)
+    boot_device, _ = proc.communicate()
+    proc = cmd_runner.run(
+        ['losetup', '-f', '--show', image_file, '--offset',
+         str(linux_offset), '--sizelimit', str(linux_size)],
+        stdout=subprocess.PIPE, as_root=True)
+    root_device, _ = proc.communicate()
+
+    return boot_device, root_device
+
+
+def calculate_partition_size_and_offset(image_file):
+    """Return the size and offset of the boot and root partitions.
+
+    Both the size and offset are in sectors.
+
+    :param image_file: A string containing the path to the image_file.
+    :return: A 4-tuple containing the offset and size of the boot partition
+        followed by the offset and size of the root partition.
+    """
+    # Here we can use parted.Device to read the partitions because we're
+    # reading from a regular file rather than a block device.  If it was a
+    # block device we'd need root rights.
+    disk = Disk(Device(image_file))
+    vfat_partition = None
+    for partition in disk.partitions:
+        if 'boot' in partition.getFlagsAsString():
+            geometry = partition.geometry
+            vfat_offset = geometry.start * 512
+            vfat_size = geometry.length * 512
+            vfat_partition = partition
+
+    assert vfat_partition is not None, (
+        "Couldn't find boot partition on %s" % image_file)
+    linux_partition = vfat_partition.nextPartition()
+    geometry = linux_partition.geometry
+    linux_offset = geometry.start * 512
+    linux_size = geometry.length * 512
+    return vfat_size, vfat_offset, linux_size, linux_offset
+
+
+def get_boot_and_root_partitions_for_media(media):
+    """Return the device files for the boot and root partitions of media.
+
+    If the given media has 2 partitions, the first is boot and the second is
+    root. If there are 3 partitions, the second is boot and third is root.
+
+    If there are any other number of partitions, ValueError is raised.
+
+    This function must only be used for block devices.
+    """
+    assert media.is_block_device, (
+        "This function must only be used for block devices")
+
+    partition_count = _get_partition_count(media)
+
+    if partition_count == 2:
+        partition_offset = 0
+    elif partition_count == 3:
+        partition_offset = 1
+    else:
+        raise ValueError(
+            "Unexpected number of partitions on %s: %d" % (
+                media.path, partition_count))
+
+    boot_partition = "%s%d" % (media.path, 1 + partition_offset)
+    root_partition = "%s%d" % (media.path, 2 + partition_offset)
+    return boot_partition, root_partition
+
+
+def _get_partition_count(media):
+    """Return the number of partitions on the given media."""
+    # We could do the same easily using python-parted but it requires root
+    # rights to read block devices, so we use UDisks here.
+    bus = dbus.SystemBus()
+    udisks = dbus.Interface(
+        bus.get_object("org.freedesktop.UDisks", "/org/freedesktop/UDisks"),
+        'org.freedesktop.UDisks')
+    device_path = udisks.get_dbus_method('FindDeviceByDeviceFile')(media.path)
+    device = bus.get_object("org.freedesktop.UDisks", device_path)
+    return device.Get(
+        device_path, 'partition-table-count',
+        dbus_interface='org.freedesktop.DBus.Properties')
+
+
+def convert_size_to_bytes(size):
+    """Convert a size string in Kbytes, Mbytes or Gbytes to bytes."""
+    unit = size[-1].upper()
+    real_size = int(size[:-1])
+    if unit == 'K':
+        real_size = real_size * 1024
+    elif unit == 'M':
+        real_size = real_size * 1024 * 1024
+    elif unit == 'G':
+        real_size = real_size * 1024 * 1024 * 1024
+    else:
+        raise ValueError("Unknown size format: %s.  Use K[bytes], M[bytes] "
+                         "or G[bytes]" % size)
+    
+    # Round the size of the raw disk image up to a multiple of 256K so it is
+    # an exact number of SD card erase blocks in length.  Otherwise Linux
+    # under qemu cannot access the last part of the card and is likely to
+    # complain that the last partition on the disk has been truncated.  This
+    # doesn't appear to work in all cases, though, as can be seen on
+    # https://bugs.launchpad.net/linux-linaro/+bug/673335.
+    if real_size % (1024 * 256):
+        cylinders = real_size / CYLINDER_SIZE
+        real_size = cylinders * CYLINDER_SIZE
+        real_size = ((((real_size - 1) / (1024 * 256)) + 1) * (1024 * 256))
+
+    return real_size
+
+
+class Media(object):
+    """A representation of the media where Linaro will be installed."""
+
+    def __init__(self, path):
+        self.path = path
+        self.is_block_device = path.startswith('/dev/')
+
+
+if __name__ == "__main__":
+    board, device, fat_size, image_size, should_create_partitions = (
+        sys.argv[1:])
+    fat_size = int(fat_size)
+    boot, root = setup_partitions(
+        board, Media(device), fat_size, image_size, should_create_partitions)
+    print "BOOTFS=%s ROOTFS=%s" % (boot, root)

=== modified file 'media_create/populate_boot.py'
--- media_create/populate_boot.py	2010-12-08 13:09:27 +0000
+++ media_create/populate_boot.py	2010-12-14 13:31:33 +0000
@@ -16,7 +16,9 @@
            '-n', name,
            '-d', img_data,
            img]
-    return cmd_runner.run(cmd, as_root=as_root, stdout=stdout)
+    proc = cmd_runner.run(cmd, as_root=as_root, stdout=stdout)
+    proc.wait()
+    return proc.returncode
 
 
 def _get_file_matching(regex):
@@ -58,24 +60,26 @@
 
 
 def install_mx51evk_boot_loader(imx_file, boot_device_or_file):
-    cmd_runner.run([
+    proc = cmd_runner.run([
         "dd",
         "if=%s" % imx_file,
         "of=%s" % boot_device_or_file,
         "bs=1024",
         "seek=1",
         "conv=notrunc"], as_root=True)
+    proc.wait()
 
 
 def install_omap_boot_loader(mlo_file, boot_disk):
-    cmd_runner.run(["cp", "-v", mlo_file, boot_disk], as_root=True)
+    cmd_runner.run(["cp", "-v", mlo_file, boot_disk], as_root=True).wait()
     # XXX: Is this really needed?
-    cmd_runner.run(["sync"])
+    cmd_runner.run(["sync"]).wait()
 
 
 def make_boot_ini(boot_script, boot_disk):
-    cmd_runner.run(
+    proc = cmd_runner.run(
         ["cp", "-v", boot_script, "%s/boot.ini" % boot_disk], as_root=True)
+    proc.wait()
 
 
 def populate_boot(board, sub_arch, load_addr, uboot_parts_dir, boot_disk,

=== modified file 'media_create/tests/fixtures.py'
--- media_create/tests/fixtures.py	2010-12-09 19:58:24 +0000
+++ media_create/tests/fixtures.py	2010-12-14 13:31:33 +0000
@@ -65,12 +65,23 @@
 
 class MockCmdRunnerPopen(object):
     """A mock for cmd_runner.Popen() which stores the args given to it."""
-    args = None
-    def __call__(self, args, **kwargs):
-        self.args = args
+    calls = None
+    def __call__(self, cmd, *args, **kwargs):
+        if self.calls is None:
+            self.calls = []
+        if isinstance(cmd, basestring):
+            all_args = [cmd]
+        else:
+            all_args = cmd
+        all_args.extend(args)
+        self.calls.append(all_args)
         self.returncode = 0
         return self
 
+    def communicate(self, input=None):
+        self.wait()
+        return '', ''
+
     def wait(self):
         return self.returncode
 

=== modified file 'media_create/tests/test_media_create.py'
--- media_create/tests/test_media_create.py	2010-12-09 20:00:37 +0000
+++ media_create/tests/test_media_create.py	2010-12-14 13:31:33 +0000
@@ -4,6 +4,7 @@
 import string
 import subprocess
 import sys
+import time
 
 from testtools import TestCase
 
@@ -12,12 +13,20 @@
 from media_create import cmd_runner
 from media_create import ensure_command
 from media_create import populate_boot
+from media_create import partitions
 from media_create.boot_cmd import create_boot_cmd
 from media_create.create_partitions import (
     create_partitions,
     run_sfdisk_commands,
     )
-from media_create.partition_size import calculate_partition_size_and_offset
+from media_create.partitions import (
+    calculate_partition_size_and_offset,
+    convert_size_to_bytes,
+    get_boot_and_root_loopback_devices,
+    get_boot_and_root_partitions_for_media,
+    Media,
+    setup_partitions,
+    )
 from media_create.populate_boot import (
     make_boot_script,
     make_uImage,
@@ -133,24 +142,28 @@
     def test_run(self):
         fixture = MockCmdRunnerPopenFixture()
         self.useFixture(fixture)
-        return_code = cmd_runner.run(['foo', 'bar', 'baz'])
-        self.assertEqual(0, return_code)
-        self.assertEqual(['foo', 'bar', 'baz'], fixture.mock.args)
+        proc = cmd_runner.run(['foo', 'bar', 'baz'])
+        self.assertEqual(0, proc.returncode)
+        self.assertEqual([['foo', 'bar', 'baz']], fixture.mock.calls)
 
     def test_run_as_root(self):
         fixture = MockCmdRunnerPopenFixture()
         self.useFixture(fixture)
         cmd_runner.run(['foo', 'bar'], as_root=True)
-        self.assertEqual(['sudo', 'foo', 'bar'], fixture.mock.args)
+        self.assertEqual([['sudo', 'foo', 'bar']], fixture.mock.calls)
 
     def test_run_succeeds_on_zero_return_code(self):
-        return_code = cmd_runner.run(['true'])
-        self.assertEqual(0, return_code)
+        proc = cmd_runner.run(['true'])
+        # Need to wait() here as we're using the real Popen.
+        proc.wait()
+        self.assertEqual(0, proc.returncode)
 
     def test_run_raises_exception_on_non_zero_return_code(self):
+        def run_and_wait():
+            proc = cmd_runner.run(['false'])
+            proc.wait()
         self.assertRaises(
-            cmd_runner.SubcommandNonZeroReturnValue,
-            cmd_runner.run, ['false'])
+            cmd_runner.SubcommandNonZeroReturnValue, run_and_wait)
 
     def test_run_must_be_given_list_as_args(self):
         self.assertRaises(AssertionError, cmd_runner.run, 'true')
@@ -181,7 +194,7 @@
             'sudo', 'mkimage', '-A', 'arm', '-O', 'linux', '-T', 'kernel',
             '-C', 'none', '-a', 'load_addr', '-e', 'load_addr', '-n', 'Linux',
             '-d', 'parts_dir/vmlinuz-*-sub_arch', 'boot_disk/uImage']
-        self.assertEqual(expected, fixture.mock.args)
+        self.assertEqual([expected], fixture.mock.calls)
 
     def test_make_uInitrd(self):
         self._mock_get_file_matching()
@@ -191,7 +204,7 @@
             'sudo', 'mkimage', '-A', 'arm', '-O', 'linux', '-T', 'ramdisk',
             '-C', 'none', '-a', '0', '-e', '0', '-n', 'initramfs',
             '-d', 'parts_dir/initrd.img-*-sub_arch', 'boot_disk/uInitrd']
-        self.assertEqual(expected, fixture.mock.args)
+        self.assertEqual([expected], fixture.mock.calls)
 
     def test_make_boot_script(self):
         self._mock_get_file_matching()
@@ -201,7 +214,7 @@
             'sudo', 'mkimage', '-A', 'arm', '-O', 'linux', '-T', 'script',
             '-C', 'none', '-a', '0', '-e', '0', '-n', 'boot script',
             '-d', 'tmp_dir/boot.cmd', 'boot_script']
-        self.assertEqual(expected, fixture.mock.args)
+        self.assertEqual([expected], fixture.mock.calls)
 
     def test_get_file_matching(self):
         prefix = ''.join(
@@ -245,32 +258,46 @@
 
 class TestCreatePartitions(TestCaseWithFixtures):
 
+    media = Media('/dev/xdz')
+
+    def setUp(self):
+        super(TestCreatePartitions, self).setUp()
+        # Stub time.sleep() as create_partitions() use that.
+        self.orig_sleep = time.sleep
+        time.sleep = lambda s: None
+
+    def tearDown(self):
+        super(TestCreatePartitions, self).tearDown()
+        time.sleep = self.orig_sleep
+
     def test_create_partitions_for_mx51evk(self):
         # For this board we create a one cylinder partition at the beginning.
         popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
         sfdisk_fixture = self.useFixture(MockRunSfdiskCommandsFixture())
 
-        create_partitions('mx51evk', '/dev/sdz', 32, 255, 63, '')
+        create_partitions('mx51evk', self.media, 32, 255, 63, '')
 
         self.assertEqual(
-            ['sudo', 'parted', '-s', '/dev/sdz', 'mklabel', 'msdos'],
-            popen_fixture.mock.args)
+            [['sudo', 'parted', '-s', self.media.path, 'mklabel', 'msdos'],
+             ['sync']],
+            popen_fixture.mock.calls)
         self.assertEqual(
-            [(',1,0xDA', 255, 63, '', '/dev/sdz'),
-             (',9,0x0C,*\n,,,-', 255, 63, '', '/dev/sdz')],
+            [(',1,0xDA', 255, 63, '', self.media.path),
+             (',9,0x0C,*\n,,,-', 255, 63, '', self.media.path)],
             sfdisk_fixture.mock.calls)
 
     def test_create_partitions_for_beagle(self):
         popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
         sfdisk_fixture = self.useFixture(MockRunSfdiskCommandsFixture())
 
-        create_partitions('beagle', '/dev/sdz', 32, 255, 63, '')
+        create_partitions('beagle', self.media, 32, 255, 63, '')
 
         self.assertEqual(
-            ['sudo', 'parted', '-s', '/dev/sdz', 'mklabel', 'msdos'],
-            popen_fixture.mock.args)
+            [['sudo', 'parted', '-s', self.media.path, 'mklabel', 'msdos'],
+             ['sync']],
+            popen_fixture.mock.calls)
         self.assertEqual(
-            [(',9,0x0C,*\n,,,-', 255, 63, '', '/dev/sdz')],
+            [(',9,0x0C,*\n,,,-', 255, 63, '', self.media.path)],
             sfdisk_fixture.mock.calls)
 
     def test_create_partitions_with_img_file(self):
@@ -278,11 +305,12 @@
         sfdisk_fixture = self.useFixture(MockRunSfdiskCommandsFixture())
 
         tempfile = self.createTempFileAsFixture()
-        create_partitions('beagle', tempfile, 32, 255, 63, '')
+        create_partitions('beagle', Media(tempfile), 32, 255, 63, '')
 
-        # popen() was not called as there's no existing partition table for
-        # us to overwrite on the image file.
-        self.assertEqual(None, popen_fixture.mock.args)
+        # Unlike the test for partitioning of a regular block device, in this
+        # case parted was not called as there's no existing partition table
+        # for us to overwrite on the image file.
+        self.assertEqual([['sync']], popen_fixture.mock.calls)
 
         self.assertEqual(
             [(',9,0x0C,*\n,,,-', 255, 63, '', tempfile)],
@@ -290,10 +318,13 @@
 
     def test_run_sfdisk_commands(self):
         tempfile = self.createTempFileAsFixture()
-        cmd_runner.run(['qemu-img', 'create', '-f', 'raw', tempfile, '10M'],
-                       stdout=subprocess.PIPE)
+        proc = cmd_runner.run(
+            ['qemu-img', 'create', '-f', 'raw', tempfile, '10M'],
+            stdout=subprocess.PIPE)
+        proc.communicate()
         stdout, stderr = run_sfdisk_commands(
-            ',1,0xDA', 5, 63, '', tempfile, as_root=False)
+            ',1,0xDA', 5, 63, '', tempfile, as_root=False,
+            stderr=subprocess.PIPE)
         self.assertIn('Successfully wrote the new partition table', stdout)
 
     def test_run_sfdisk_commands_raises_on_non_zero_returncode(self):
@@ -301,21 +332,132 @@
         self.assertRaises(
             cmd_runner.SubcommandNonZeroReturnValue,
             run_sfdisk_commands,
-            ',1,0xDA', 5, 63, '', tempfile, as_root=False)
-
-
-class TestCalculatePartitionSizeAndOffset(TestCaseWithFixtures):
-
-    def test_foo(self):
-        tempfile = self.createTempFileAsFixture()
-        cmd_runner.run(['qemu-img', 'create', '-f', 'raw', tempfile, '10M'],
-                       stdout=subprocess.PIPE)
-        stdout, stderr = run_sfdisk_commands(
-            ',1,0x0C,*\n,,,-', 5, 63, '', tempfile, as_root=False)
-        self.assertIn('Successfully wrote the new partition table', stdout)
-
+            ',1,0xDA', 5, 63, '', tempfile, as_root=False,
+            stderr=subprocess.PIPE)
+
+
+class TestPartitionSetup(TestCaseWithFixtures):
+
+    def setUp(self):
+        super(TestPartitionSetup, self).setUp()
+        # Stub time.sleep() as create_partitions() use that.
+        self.orig_sleep = time.sleep
+        time.sleep = lambda s: None
+
+    def tearDown(self):
+        super(TestPartitionSetup, self).tearDown()
+        time.sleep = self.orig_sleep
+
+    def test_convert_size_in_kbytes_to_bytes(self):
+        self.assertEqual(512 * 1024, convert_size_to_bytes('512K'))
+
+    def test_convert_size_in_mbytes_to_bytes(self):
+        self.assertEqual(100 * 1024**2, convert_size_to_bytes('100M'))
+
+    def test_convert_size_in_gbytes_to_bytes(self):
+        self.assertEqual(12 * 1024**3, convert_size_to_bytes('12G'))
+
+    def test_convert_size_in_kbytes_to_bytes_rounds_to_256k_multiple(self):
+        # See comment in convert_size_to_bytes as to why we need to do this.
+        self.assertEqual(
+            3891 * (1024 * 256), convert_size_to_bytes('1000537K'))
+
+    def test_calculate_partition_size_and_offset(self):
+        tempfile = self._create_qemu_img_with_partitions(',1,0x0C,*\n,,,-')
         vfat_size, vfat_offset, linux_size, linux_offset = (
             calculate_partition_size_and_offset(tempfile))
         self.assertEqual(
             [129024L, 32256L, 10321920L, 161280L],
             [vfat_size, vfat_offset, linux_size, linux_offset])
+
+    def test_get_boot_and_root_partitions_for_media_with_2_partitions(self):
+        self.useFixture(MockSomethingFixture(
+            partitions, '_get_partition_count', lambda media: 2))
+        tempfile = self._create_qemu_img_with_partitions(',1,0x0C,*\n,,,-')
+        media = Media(tempfile)
+        # Pretend the image file is a block device, or else
+        # get_boot_and_root_partitions_for_media will choke.
+        media.is_block_device = True
+        self.assertEqual(
+            ("%s%d" % (tempfile, 1), "%s%d" % (tempfile, 2)),
+            get_boot_and_root_partitions_for_media(media))
+
+    def test_get_boot_and_root_partitions_for_media_with_3_partitions(self):
+        self.useFixture(MockSomethingFixture(
+            partitions, '_get_partition_count', lambda media: 3))
+        tempfile = self._create_qemu_img_with_partitions(
+            ',1,0xDA\n,1,0x0C,*\n,,,-')
+        media = Media(tempfile)
+        # Pretend the image file is a block device, or else
+        # get_boot_and_root_partitions_for_media will choke.
+        media.is_block_device = True
+        self.assertEqual(
+            ("%s%d" % (tempfile, 2), "%s%d" % (tempfile, 3)),
+            get_boot_and_root_partitions_for_media(media))
+
+    def _create_qemu_img_with_partitions(self, sfdisk_commands):
+        tempfile = self.createTempFileAsFixture()
+        proc = cmd_runner.run(
+            ['qemu-img', 'create', '-f', 'raw', tempfile, '10M'],
+            stdout=subprocess.PIPE)
+        proc.communicate()
+        stdout, stderr = run_sfdisk_commands(
+            sfdisk_commands, 5, 63, '', tempfile, as_root=False,
+            # Throw away stderr as sfdisk complains a lot when operating on a
+            # qemu image.
+            stderr=subprocess.PIPE)
+        self.assertIn('Successfully wrote the new partition table', stdout)
+        return tempfile
+
+    def test_get_boot_and_root_loopback_devices(self):
+        tempfile = self._create_qemu_img_with_partitions(',1,0x0C,*\n,,,-')
+        popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
+        # We can't test the return value of get_boot_and_root_loopback_devices
+        # because it'd require running losetup as root, so we just make sure
+        # it calls losetup correctly.
+        get_boot_and_root_loopback_devices(tempfile)
+        self.assertEqual(
+            [['sudo', 'losetup', '-f', '--show', tempfile, '--offset',
+              '32256', '--sizelimit', '129024'],
+             ['sudo', 'losetup', '-f', '--show', tempfile, '--offset',
+              '161280', '--sizelimit', '10321920']],
+            popen_fixture.mock.calls)
+
+    def test_setup_partitions_for_image_file(self):
+        # In practice we could pass an empty image file to setup_partitions,
+        # but here we mock Popen() and thanks to that the image is not setup
+        # (via qemu-img) inside setup_partitions.  That's why we pass an
+        # already setup image file.
+        tempfile = self._create_qemu_img_with_partitions(',1,0x0C,*\n,,,-')
+        popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
+        setup_partitions('beagle', Media(tempfile), 32, '2G', 'yes')
+        self.assertEqual(
+             # This is the call that would create the image file.
+            [['qemu-img', 'create', '-f', 'raw', tempfile, '2G'],
+             # This call would partition the image file.
+             ['sudo', 'sfdisk', '-D', '-H', '255', '-S', '63', '-C', '261',
+              tempfile],
+             # Make sure changes are written to disk.
+             ['sync'],
+             # Register boot/root loopback devices so that we can just copy
+             # their contents over and have it written to the image file.
+             ['sudo', 'losetup', '-f', '--show', tempfile, '--offset',
+              '32256', '--sizelimit', '129024'],
+             ['sudo', 'losetup', '-f', '--show', tempfile, '--offset',
+              '161280', '--sizelimit', '10321920']],
+            popen_fixture.mock.calls)
+
+    def test_setup_partitions_for_block_device(self):
+        self.useFixture(MockSomethingFixture(
+            partitions, '_get_partition_count', lambda media: 2))
+        tempfile = self._create_qemu_img_with_partitions(',1,0x0C,*\n,,,-')
+        media = Media(tempfile)
+        # Pretend our tempfile is a block device.
+        media.is_block_device = True
+        popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
+        setup_partitions('beagle', media, 32, '2G', 'yes')
+        self.assertEqual(
+            [['sudo', 'parted', '-s', tempfile, 'mklabel', 'msdos'],
+             ['sudo', 'sfdisk', '-D', '-H', '255', '-S', '63', tempfile],
+             ['sync']],
+            popen_fixture.mock.calls)

=== added file 'tests/integration.txt'
--- tests/integration.txt	1970-01-01 00:00:00 +0000
+++ tests/integration.txt	2010-12-14 13:31:33 +0000
@@ -0,0 +1,25 @@
+A few integration tests we can run while the migration to python is not
+complete.  They probably require root or access to specific block devices so
+they are not meant to be automated.
+
+    # This should print nothing to stdout but will create a binary/ dir under
+    # the current directory.
+    >>> python -m media_create.unpack_binary_tarball <path-to-binary-tarball>
+
+    # This will remove the binary/ dir created above
+    >>> python -m media_create.remove_binary_dir
+
+    # Partition (for real!) /dev/sdb for a beagle board and return the devices
+    # for the boot and root partitions.
+    >>> python -m media_create.partitions beagle /dev/sdb 32 2G yes
+    Checking that no-one is using this disk right now
+    ...
+    BOOTFS=/dev/sdb1 ROOTFS=/dev/sdb2
+
+    # Partition /tmp/beagle.img for a beagle board and return the loopback
+    # devices for the boot and root partitions.
+    >>> python -m media_create.partitions beagle /tmp/beagle.img 32 2G yes
+    Warning: /tmp/beagle.img is not a block device
+    ...
+    BOOTFS=/dev/loop0
+    ROOTFS=/dev/loop1