Scott,

There was a newer revision checked in last night but you commented on the
old revision.  Looking over your comments.  As far as tabs, I am using
emacs and it is using 4-spaces so your comment on the tab indentation seems
odd and I have checked the file and everything lines up so maybe launchpad
shows it like that?  I will update branch and push so you can have a look
at it on Monday.


On Fri, Jul 18, 2014 at 6:05 AM, Scott Moser <email address hidden> wrote:

> I'll take a look further next week,
> but some nitpick comments inline.
>
> make sure you follow the code conventions in tools/common (4 space
> indentation).
>
>
> Diff comments:
>
> > === modified file 'curtin/commands/block_meta.py'
> > --- curtin/commands/block_meta.py     2014-06-19 11:51:08 +0000
> > +++ curtin/commands/block_meta.py     2014-07-15 18:09:58 +0000
> > @@ -32,7 +32,7 @@
> >         'metavar': 'DEVICE', 'default': None, }),
> >       ('--fstype', {'help': 'root filesystem type',
> >                     'choices': ['ext4', 'ext3'], 'default': 'ext4'}),
> > -     ('mode', {'help': 'meta-mode to use', 'choices': ['raid0',
> 'simple']}),
> > +     ('mode', {'help': 'meta-mode to use', 'choices': ['raid0',
> 'simple', 'simple-boot']}),
> >       )
> >  )
> >
> > @@ -41,6 +41,8 @@
> >      # main entry point for the block-meta command.
> >      if args.mode == "simple":
> >          meta_simple(args)
> > +    elif args.mode == "simple-boot":
> > +        meta_simple_boot(args)
> >      else:
> >          raise NotImplementedError("mode=%s is not implemenbed" %
> args.mode)
> >
> > @@ -131,6 +133,80 @@
> >      return 0
> >
> >
> > +def meta_simple_boot(args):
> > +    """Similar to meta_simple but it also creates an extra /boot
> partition.
> > +    This is needed from some instances of u-boot.
> > +    """
> > +    state = util.load_command_environment()
> > +
> > +    cfg = util.load_command_config(args, state)
> > +
> > +    devices = args.devices
> > +    if devices is None:
> > +        devices = cfg.get('block-meta', {}).get('devices', [])
> > +
> > +    # Remove duplicates but maintain ordering.
> > +    devices = list(OrderedDict.fromkeys(devices))
> > +
> > +    if len(devices) == 0:
> > +        devices = block.get_installable_blockdevs()
> > +        LOG.warn("simple-boot mode, no devices given. unused list: %s",
> devices)
> > +
> > +    if len(devices) > 1:
> > +        if args.devices is not None:
> > +            LOG.warn("simple-boot mode but multiple devices given. "
> > +                     "using first found")
> > +        available = [f for f in devices
> > +                     if block.is_valid_device(f)]
> > +        target = sorted(available)[0]
> > +        LOG.warn("mode is 'simple-boot'. multiple devices given. using
> '%s' "
> > +                 "(first available)", target)
> > +    else:
> > +        target = devices[0]
> > +
> > +    if not block.is_valid_device(target):
> > +        raise Exception("target device '%s' is not a valid device" %
> target)
> > +
> > +    (devname, devnode) = block.get_dev_name_entry(target)
> > +
> > +    LOG.info("installing in simple-boot mode to '%s'", devname)
> > +
> > +    sources = cfg.get('sources', {})
> > +    dd_images = util.get_dd_images(sources)
> > +    if len(dd_images):
> > +        # we have at least one dd-able image
> > +        # we will only take the first one
> > +        rootdev = write_image_to_disk(dd_images[0], devname)
> > +        util.subp(['mount', rootdev, state['target']])
> > +        return 0
> > +
> > +    if util.is_uefi_bootable():
> > +        logtime(
> > +            "partition --format uefi --boot %s" % devnode,
> > +            util.subp, ("partition", "--format", "uefi", "--boot",
> devnode))
> > +    else:
> > +        logtime(
> > +            "partition --boot %s" % devnode,
> > +            util.subp, ("partition", "--boot", devnode))
> > +
> > +    bootdev = devnode + "1"
> > +    rootdev = devnode + "2"
> > +
> > +    cmd = ['mkfs.%s' % args.fstype, '-q', '-L', 'cloudimg-bootfs',
> bootdev]
> > +    logtime(' '.join(cmd), util.subp, cmd)
> > +    util.subp(['mount', bootdev, state['target']])
> > +
> > +    cmd = ['mkfs.%s' % args.fstype, '-q', '-L', 'cloudimg-rootfs',
> rootdev]
> > +    logtime(' '.join(cmd), util.subp, cmd)
> > +    util.subp(['mount', rootdev, state['target']])
> > +
> > +    with open(state['fstab'], "w") as fp:
> > +        fp.write("LABEL=%s /boot %s defaults 0 0\n" %
> ('cloudimg-bootfs', args.fstype))
> > +        fp.write("LABEL=%s / %s defaults 0 0\n" % ('cloudimg-rootfs',
> args.fstype))
> > +
> > +    return 0
> > +
> > +
> >  def POPULATE_SUBCMD(parser):
> >      populate_one_subcmd(parser, CMD_ARGUMENTS, block_meta)
> >
> >
> > === modified file 'curtin/commands/install.py'
> > --- curtin/commands/install.py        2014-06-08 13:52:36 +0000
> > +++ curtin/commands/install.py        2014-07-15 18:09:58 +0000
> > @@ -29,13 +29,15 @@
> >
> >  from . import populate_one_subcmd
> >
> > +
> >  CONFIG_BUILTIN = {
> >      'sources': {},
> >      'stages': ['early', 'partitioning', 'network', 'extract',
> 'curthooks',
> >                 'hook', 'late'],
> >      'extract_commands': {'builtin': ['curtin', 'extract']},
> >      'hook_commands': {'builtin': ['curtin', 'hook']},
> > -    'partitioning_commands': {'builtin': ['curtin', 'block-meta',
> 'simple']},
> > +    'partitioning_commands': {
> > +        'builtin': ['curtin', 'block-meta',
> util.partitioning_command()]},
> >      'curthooks_commands': {'builtin': ['curtin', 'curthooks']},
> >      'late_commands': {'builtin': []},
> >      'network_commands': {'builtin': ['curtin', 'net-meta', 'auto']},
> >
> > === modified file 'curtin/util.py'
> > --- curtin/util.py    2014-06-24 15:06:00 +0000
> > +++ curtin/util.py    2014-07-15 18:09:58 +0000
> > @@ -17,6 +17,7 @@
> >
> >  import errno
> >  import os
> > +import platform
> >  import shutil
> >  import subprocess
> >  import sys
> > @@ -503,4 +504,14 @@
> >              src.append(sources[i]['uri'])
> >      return src
> >
> > +
> > +def partitioning_command():
> > +    """
> > +    return the meta-block command for paritioning
> > +    """
> > +    mach = platform.machine()
> > +    if mach == 'aarch64':
> > +        return 'simple-boot'
> > +    return 'simple'
> > +
> >  # vi: ts=4 expandtab syntax=python
> >
> > === modified file 'helpers/common'
> > --- helpers/common    2014-04-21 16:18:39 +0000
> > +++ helpers/common    2014-07-15 18:09:58 +0000
> > @@ -12,9 +12,10 @@
> >     destroy any partition table that might be there already.
> >
> >     options:
> > +     -E | --end E      end the partition at E (unit 1k bytes)
> >       -f | --format F   use partition table format F. [mbr, gpt, uefi]
> >                         default gpt
> > -     -E | --end E      end the partition at E (unit 1k bytes)
> > +     -b | --boot       create a 512MiB boot partition
> >  EOF
> >      [ $# -eq 0 ] || echo "$@"
> >  }
> > @@ -60,8 +61,8 @@
> >  }
> >
> >  pt_gpt() {
> > -    local target="$1" end=${2:-""} size="" s512="" ptype="L"
> > -    local start="2048" pt1size="" maxend=""
> > +    local target="$1" end=${2:-""} boot="$3" size="" s512="" ptype="L"
> > +    local start="2048" rootsize="" bootsize="1048576" maxend=""
> >      local isblk=false
> >      getsize "$target" ||
> >          { error "failed to get size of $target"; return 1; }
> > @@ -71,15 +72,32 @@
> >      else
> >          end=$(($end/512))
> >      fi
> > -    maxend=$((($size/512)-2048))
> > +
> > +    # 512MiB = 536870912 bytes / 512 bytes per sector = 1048576 sectors
> > +    if [ -n "$boot" ]; then
> > +        maxend=$((($size/512)-$start-$bootsize))
> > +     if [ $maxend -lt 0 ]; then
> > +         { error "Disk is not big enough for /boot partition on
> $target";
> > +             return 1; }
> > +     fi
>
> some bad code indentation here.  indentation is 4 spaces. make sure not to
> use tabs. set your vi to:
>   set modeline modelines=3
> and it will handle converting tab correctly for you.
>
> also, the [ $maxend -lt 0 ]
> is a bad check, as maxend might be "" at this point, and will fail for
> that reason.
>
> > +    else
> > +        maxend=$((($size/512)-$start))
> > +    fi
> >      [ "$end" -gt "$maxend" ] && end="$maxend"
> >      debug 1 "maxend=$maxend end=$end size=$size"
> >
> >      [ -b "$target" ] && isblk=true
> >
> > -    sgdisk --new "15:2048:+1M" --typecode=15:ef02 \
> > -           --new "1::$end"     --typecode=1:8300 "$target" ||
> > -        { error "failed to gpt partition $target"; return 1; }
> > +    if [ -n "$boot" ]; then
> > +        sgdisk --new "15:$start:+1M" --typecode=15:ef02 \
> > +            --new "1:+1M:+513M" --typecode=1:8300 \
> > +               --new "2::$end"     --typecode=2:8300 "$target" ||
> > +            { error "failed to gpt partition $target"; return 1; }
> > +    else
> > +        sgdisk --new "15:2048:+1M" --typecode=15:ef02 \
> > +               --new "1::$end"     --typecode=1:8300 "$target" ||
> > +            { error "failed to gpt partition $target"; return 1; }
> > +    fi
> >
> >      if $isblk; then
> >          blockdev --rereadpt "$target"
> > @@ -88,12 +106,16 @@
> >              { error "no partition found ${target}1"; return 1; }
> >          [ -b "${target}15" ] ||
> >              { error "no partition found ${target}15"; return 1; }
> > +     if [ -n "$boot" ]; then
> > +            [ -b "${target}2" ] ||
> > +                { error "no partition found ${target}2"; return 1; }
> > +     fi
> >      fi
> >  }
> >
> >  pt_uefi() {
> > -    local target="$1" end=${2:-""} size="" s512="" ptype="L"
> > -    local start="2048" pt1size="" maxend=""
> > +    local target="$1" end=${2:-""} boot="$3" size="" s512="" ptype="L"
> > +    local start="2048" rootsize="" bootsize="1048576" maxend=""
> >      local isblk=false
> >      getsize "$target" ||
> >          { error "failed to get size of $target"; return 1; }
> > @@ -103,16 +125,33 @@
> >      else
> >          end=$(($end/512))
> >      fi
> > -    maxend=$((($size/512)-2048))
> > +
> > +    # 512MiB = 536870912 bytes / 512 bytes per sector = 1048576 sectors
> > +    if [ -n "$boot" ]; then
> > +        maxend=$((($size/512)-$start-$bootsize))
> > +     if [ $maxend -lt 0 ]; then
> > +         { error "Disk is not big enough for /boot partition on
> $target";
> > +             return 1; }
> > +     fi
> > +    else
> > +     maxend=$((($size/512)-$start))
> > +    fi
> >      [ "$end" -gt "$maxend" ] && end="$maxend"
> >      debug 1 "maxend=$maxend end=$end size=$size"
> >
> >      [ -b "$target" ] && isblk=true
> >
> >      # Part 15 is the UEFI partition, part 1 is root
> > -    sgdisk --new "15:2048:+512M" --typecode=15:ef00 "$target" \
> > -           --new "1::$end" --typecode=1:8300 "$target" ||
> > -        { error "failed to sgdisk for uefi to $target"; return 1; }
> > +    if [ -n "$boot" ]; then
> > +        sgdisk --new "15:$start:+512M" --typecode=15:ef00 "$target" \
> > +            --new "1:+512M:+1024M" --typecode=1:8300 \
> > +               --new "2::$end" --typecode=2:8300 "$target" ||
> > +            { error "failed to sgdisk for uefi to $target"; return 1; }
> > +    else
> > +        sgdisk --new "15:$start:+512M" --typecode=15:ef00 "$target" \
> > +               --new "1::$end" --typecode=1:8300 "$target" ||
> > +            { error "failed to sgdisk for uefi to $target"; return 1; }
> > +    fi
> >
> >      if $isblk; then
> >          blockdev --rereadpt "$target"
> > @@ -121,6 +160,10 @@
> >              { error "no partition found ${target}1"; return 1; }
> >          [ -b "${target}15" ] ||
> >              { error "no partition found ${target}15"; return 1; }
> > +     if [ -n "$boot" ]; then
> > +            [ -b "${target}2" ] ||
> > +                { error "no partition found ${target}2"; return 1; }
> > +     fi
> >      fi
> >
> >      mkfs -t vfat -F 32 -n uefi-boot ${target}15 ||
> > @@ -129,8 +172,8 @@
> >
> >
> >  pt_mbr() {
> > -    local target="$1" end=${2:-""} size="" s512="" ptype="L"
> > -    local start="2048" pt1size="" maxsize="4294967296"
> > +    local target="$1" end=${2:-""} boot="$3" size="" s512="" ptype="L"
> > +    local start="2048" rootsize="" bootsize="1048576"
> maxsize="4294967296"
> >      local isblk=false
> >      getsize "$target" ||
> >          { error "failed to get size of $target"; return 1; }
> > @@ -142,30 +185,61 @@
> >      else
> >          s512=$(($size/512))
> >      fi
> > +    # 512MiB = 536870912 bytes / 512 bytes per sector = 1048576 sectors
> >      if [ -n "$end" ]; then
> > -        pt1size=$(((end/512)-2048))
> > +     if [ -n "$boot" ]; then
> > +            rootsize=$(((end/512)-$start-$bootsize))
> > +     else
> > +         rootsize=$(((end/512)-$start))
> > +     fi
> >      else
> > -        pt1size=$((s512-2048))
> > +     if [ -n "$boot" ]; then
> > +            rootsize=$((s512-Sstart-$bootsize))
> > +     else
> > +         rootsize=$((s512-$start))
> > +     fi
> >      fi
> >
> >      [ -b "$target" ] && isblk=true
> >
> >      # interact with sfdisk in units of 512 bytes (--unit S)
> >      # we start all partitions at 2048 of those (1M)
> > -    local sfdisk_out="" sfdisk_in="2048,$pt1size,$ptype,*" sfdisk_cmd=""
> > -    sfdisk_cmd=( sfdisk --no-reread --force --Linux --unit S "$target" )
> > -    debug 1 "sfdisking with: echo '$sfdisk_in' | ${sfdisk_cmd[*]}"
> > -    sfdisk_out=$(echo "$sfdisk_in" | "${sfdisk_cmd[@]}" 2>&1)
> > -    ret=$?
> > -    [ $ret -eq 0 ] || {
> > -        error "failed to partition $target [${sfdisk_out}]";
> > -        return 1;
> > -    }
> > +    if [ -n "$boot" ]; then
> > +     local rootstart=$start+$bootsize
> > +     # Create /boot and / partition
> > +     local sfdisk_out=""
> > +     local
> sfdisk_in="EOF\n$start,$bootsize,$ptype\n$rootstart,$rootsize,$ptype\nEOF"
> > +     local sfdisk_cmd=""
> > +     sfdisk_cmd=( sfdisk --no-reread --force --Linux --unit S "$target"
> )
> > +     debug 1 "sfdisking with: echo '$sfdisk_in' | ${sfdisk_cmd[*]}"
> > +     sfdisk_out=$(printf "$sfdisk_in" | "${sfdisk_cmd[@]}" 2>&1)
> > +
> > +     ret=$?
> > +     [ $ret -eq 0 ] || {
> > +            error "failed to partition $target [${sfdisk_out}]";
> > +            return 1;
> > +     }
> > +    else
> > +     local sfdisk_out="" sfdisk_in="$start,$rootsize,$ptype,*"
> sfdisk_cmd=""
> > +     sfdisk_cmd=( sfdisk --no-reread --force --Linux --unit S "$target"
> )
> > +     debug 1 "sfdisking with: echo '$sfdisk_in' | ${sfdisk_cmd[*]}"
> > +     sfdisk_out=$(echo "$sfdisk_in" | "${sfdisk_cmd[@]}" 2>&1)
> > +
> > +     ret=$?
> > +     [ $ret -eq 0 ] || {
> > +            error "failed to partition $target [${sfdisk_out}]";
> > +            return 1;
> > +     }
> > +    fi
> >      if $isblk; then
> >          blockdev --rereadpt "$target"
> >          udevadm settle
> >          [ -b "${target}1" ] ||
> >              { error "no partition found ${target}1"; return 1; }
> > +     if [ -n "$boot" ]; then
> > +            [ -b "${target}2" ] ||
> > +                { error "no partition found ${target}2"; return 1; }
> > +     fi
>
> if [ -n "$boot" -a ! -b "${target}2" ]; then
>   error "no partition found"
>   return 1
> fi
>
> >      fi
> >      out=$(wipefs "--offset=$(($start*512))" "$target" 2>&1) || {
> >          error "$out";
> > @@ -175,22 +249,23 @@
> >  }
> >
> >  partition_main() {
> > -    local short_opts="hE:f:v"
> > -    local long_opts="help,end:,format:,verbose"
> > +    local short_opts="hE:f:bv"
> > +    local long_opts="help,end:,format:,boot,verbose"
> >      local getopt_out=$(getopt --name "${0##*/}" \
> >          --options "${short_opts}" --long "${long_opts}" -- "$@") &&
> >          eval set -- "${getopt_out}" ||
> >          { partition_main_usage 1>&2; return 1; }
> >
> >      local cur="" next=""
> > -    local format="mbr" target="" end=""
> > +    local format="gpt" boot="" target="" end=""
> >
> >      while [ $# -ne 0 ]; do
> >          cur="$1"; next="$2";
> >          case "$cur" in
> >              -h|--help) partition_main_usage ; exit 0;;
> > +            -E|--end) end=$next; shift;;
> >              -f|--format) format=$next; shift;;
> > -            -E|--end) end=$next; shift;;
> > +         -b|--boot) boot=true;;
> >              -v|--verbose) VERBOSITY=$((${VERBOSITY}+1));;
> >              --) shift; break;;
> >          esac
> > @@ -220,11 +295,11 @@
> >          { error "wiping $target failed"; return 1; }
> >
> >      if [ "$format" = "mbr" ]; then
> > -        pt_mbr "$target" "$end"
> > +        pt_mbr "$target" "$end" "$boot"
> >      elif [ "$format" = "gpt" ]; then
> > -        pt_gpt "$target" "$end"]
> > +        pt_gpt "$target" "$end" "$boot"
> >      elif [ "$format" = "uefi" ]; then
> > -        pt_uefi "$target" "$end"
> > +        pt_uefi "$target" "$end" "$boot"
> >      fi
> >
> >      trap cleanup EXIT
> >
>
>
> --
>
> https://code.launchpad.net/~newell-jensen/curtin/add-arm64-support/+merge/226196
> You are the owner of lp:~newell-jensen/curtin/add-arm64-support.
>