Preserves file system metadata between new build and latest shipping

align_rootfs

Index: align_rootfs
diff --git a/align_rootfs b/align_rootfs
new file mode 100755
index 0000000000000000000000000000000000000000..248a7601f1457e74a104469c93e154468bd1fe9b
--- /dev/null
+++ b/align_rootfs
@@ -0,0 +1,264 @@
+#!/bin/bash
+
+# Copyright (c) 2009 The Chromium OS Authors. All rights reserved.

[petkov, 2010/12/22 22:45:03]

2010

[gauravsh, 2010/12/22 22:55:27]

2010 (or rather 2011 if you end up submitting next year :) )

[thieule, 2010/12/23 19:26:28]

Done.

[thieule, 2010/12/23 19:26:28]

Done.

+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+# Script to preserve the on-disk file layout of the specified image and
+# the latest shipping image.
+
+# Load common constants.  This should be the first executable line.
+# The path to common.sh should be relative to your script's location.
+. "$(dirname "$0")/common.sh"
+. "$(dirname "$0")/chromeos-common.sh"
+
+# Script must be run inside the chroot because we need to use
+# cros_make_image_bootable which requires this.

[adlr, 2010/12/22 22:49:50]

i suspect this might be a problem for the build bots. i would put in a TODO to
fix this and maybe look into getting cros_make_image_bootable working outside
the chroot

[thieule, 2010/12/23 19:26:28]

Done.

+assert_inside_chroot
+
+locate_gpt
+get_default_board
+
+# Flags.
+DEFINE_string board "${DEFAULT_BOARD}" \
+  "The board of the latest shipping image to use."
+DEFINE_string image "" \
+  "The image that needs to be aligned to the latest shipping image."
+
+# Parse command line.

[petkov, 2010/12/22 22:45:03]

you should do the flag processing inside main

[thieule, 2010/12/23 19:26:28]

Done.

+FLAGS "$@" || exit 1
+eval set -- "${FLAGS_ARGV}"
+
+# Only now can we die on error.  shflags functions leak non-zero error codes,
+# so will die prematurely if 'set -e' is specified before now.
+set -e
+
+# Make sure we have the required parameters.
+if [ -z "${FLAGS_board}" ] ; then
+  error "--board is required."

[petkov, 2010/12/22 22:45:03]

die instead of error and no exit

[thieule, 2010/12/23 19:26:28]

Done.

+  exit 1
+fi
+
+if [ ! -f "${FLAGS_image}" ] ; then
+  error "--image is required."

[petkov, 2010/12/22 22:45:03]

die

also, did you mean to say that the image file does not exist?

[gauravsh, 2010/12/22 22:55:27]

the error displayed here is not accurate - since -f checks for file existence.
suggest you move the -z check on flags to the check earlier, and check for file
existence here.

[thieule, 2010/12/23 19:26:28]

Done.

[thieule, 2010/12/23 19:26:28]

Done.

+  exit 1
+fi
+
+HARDWARE_CLASS=
+IMAGE_SEARCH_STRING=
+RELEASE_URL=
+RELEASE_ALT_URL=
+
+LATEST_SHIPPING_VERSION=
+
+declare -a cleanup_actions
+
+add_cleanup_action() {

[gauravsh, 2010/12/22 22:55:27]

this trap setup and cleanup will conflict with
vboot_reference/scripts/image_signing/common.sh, maybe it can be made a part of
common.sh and the cleanup function there?

[thieule, 2010/12/23 19:26:28]

Done.

+  # Add the clean up action to the end of the array.
+  # During clean up, the actions are executed in the reverse order of when
+  # they were added.
+  cleanup_actions[${#cleanup_actions[*]}]=$1
+}
+
+cleanup() {
+  set +e
+
+  # Perform clean up actions in reverse order.
+  local num_actions=${#cleanup_actions[*]}
+  while [ $num_actions -gt 0 ] ; do
+    eval "${cleanup_actions[$num_actions-1]}" > /dev/null 2>&1
+    unset cleanup_actions[$num_actions-1]
+    num_actions=${#cleanup_actions[*]}
+  done
+
+  set -e
+}
+
+initialize() {
+  # Set up environment variables specific to the board
+  if [ ${FLAGS_board} == "x86-mario" ] ; then

[petkov, 2010/12/22 22:45:03]

this is a bit ugly. maybe it's better to remove the board flag and make the
hardware class a required argument.

[gauravsh, 2010/12/22 22:55:27]

use '=' here

[thieule, 2010/12/23 19:26:28]

Done.

[thieule, 2010/12/23 19:26:28]

Done.

+    HARDWARE_CLASS="IEC MARIO PONY 6101"

[gauravsh, 2010/12/22 22:55:27]

this could be better called HARDWARE_ID.

[gauravsh, 2010/12/22 22:55:27]

You might want to parameterize this now - as an optional command line argument.

[thieule, 2010/12/23 19:26:28]

Done.

[thieule, 2010/12/23 19:26:28]

Making this a required command line arg per petkov suggestion.

+    IMAGE_SEARCH_STRING="*SSD_MP_SIGNED.bin"
+    RELEASE_URL="http://chromeos-images/chromeos-official/dev-channel/x86-mario"
+    RELEASE_ALT_URL="${RELEASE_URL}-rc"
+  else
+    error "Board ${FLAGS_board} not supported"

[petkov, 2010/12/22 22:45:03]

die

[thieule, 2010/12/23 19:26:28]

Done.

+    exit 1
+  fi
+}
+
+get_latest_shipping_version() {

[gauravsh, 2010/12/22 22:55:27]

comment

[thieule, 2010/12/23 19:26:28]

Done.

+  info "Pinging Omaha for the latest shipping version."

[petkov, 2010/12/22 22:45:03]

it might be easier/better to go through the recovery flow instead of omaha --
the recovery URL should point to the latest recovery image which is just a
gzipped image file. then you wouldn't need all the code that looks for the
filename, download URL, etc.

[thieule, 2010/12/23 19:26:28]

Is the recovery image always the latest shipping image?

+  local auserver_url="https://tools.google.com/service/update2"
+  local au_request_file=$(mktemp "/tmp/align_rootfs_au_request.XXXX")
+  add_cleanup_action "sudo rm -f \"${au_request_file}\""
+  cat > "${au_request_file}" << EOF
+<?xml version="1.0" encoding="UTF-8"?>
+<o:gupdate xmlns:o="http://www.google.com/update2/request"
+   version="ChromeOSUpdateEngine-0.1.0.0"
+   updaterversion="ChromeOSUpdateEngine-0.1.0.0" protocol="2.0" ismachine="1">
+    <o:os version="Indy" platform="Chrome OS" sp="ForcedUpdate_i686"></o:os>
+    <o:app appid="{87efface-864d-49a5-9bb3-4b050a7c227a}"

[petkov, 2010/12/22 22:45:03]

you're hardcoding an AppId that corresponds to Mario hardware IDs. maybe make it
a flag with the right default value.

[thieule, 2010/12/23 19:26:28]

Done.

+       version="ForcedUpdate" lang="en-US" track="dev-channel"

[petkov, 2010/12/22 22:45:03]

if you use version="0.0.0.0" you should get the latest version. I'm not sure
what ForcedUpdate would give you from the live Omaha server.

[thieule, 2010/12/23 19:26:28]

Done.

+       hardware_class="${HARDWARE_CLASS}" delta_okay="true">
+        <o:ping a="-1" r="-1"></o:ping>

[petkov, 2010/12/22 22:45:03]

remove this ping otherwise you'd be counted as an active user every time you run
the script

[thieule, 2010/12/23 19:26:28]

Done.

+        <o:updatecheck></o:updatecheck>
+    </o:app>
+</o:gupdate>
+EOF
+
+  LATEST_SHIPPING_VERSION=$(wget -q --header="Content-Type: text/xml" \

[petkov, 2010/12/22 22:45:03]

why is this a global? shouldn't you return this as a value from the function
instead?

[thieule, 2010/12/23 19:26:28]

Done.

+    --post-file="${au_request_file}" -O - ${auserver_url} | \
+    sed 's/.*\(ChromeOSVersion="\)\([0-9\.]*\)"\(.*\)/\2/')
+}
+
+download_image() {

[gauravsh, 2010/12/22 22:55:27]

add comment

[thieule, 2010/12/23 19:26:28]

Done.

+  # Since we don't know the exact name, we'll just download recursively based
+  # on a wildcard. Then we'll # rename that download file to the desired output
+  # file. It's important that the IMAGE_SEARCH_STRING matches only one file.
+  local url=$1
+  local output_file=$2
+  local retcode=0
+
+  local download_dir=$(mktemp -d "/tmp/align_rootfs_download_dir.XXXX")
+  add_cleanup_action "sudo rm -rf \"${download_dir}\""
+
+  wget -A "${IMAGE_SEARCH_STRING}" --progress=bar -r -l1 -nd \
+    -P "${download_dir}" ${url} || retcode=$?

[adlr, 2010/12/22 22:49:50]

cool trick to get the retcode! I'll have to remember that one

+  if [ ${retcode} -eq 0 ] ; then
+    mv -f "${download_dir}"/* "${output_file}"
+  else
+    return ${retcode}
+  fi
+}
+
+get_shipping_image() {
+  # The image may reside in one of two URLs. We don't know exactly which one
+  # so we'll have to try both.
+  local version=$1
+  local image=$2
+  local url="${RELEASE_URL}/${version}"
+  download_image "${url}" "${image}" || retcode=$?
+  if [ ${retcode} -gt 0 ] ; then
+    url="${RELEASE_ALT_URL}/${version}"
+    download_image "${url}" "${image}"
+  fi
+}
+
+extract_root_fs() {
+  local image=$1
+  local root_fs=$2
+  local root_fs_offset=$(partoffset "${image}" 3)
+  local root_fs_count=$(partsize "${image}" 3)
+  dd if="${image}" of="${root_fs}" bs=512 skip=${root_fs_offset} \
+    count=${root_fs_count}
+}
+
+copy_root_fs() {

[gauravsh, 2010/12/22 22:55:27]

add comment about the arguments and what the function does

[thieule, 2010/12/23 19:26:28]

Done.

+  local src_image=$1
+  local dst_root_fs=$2
+
+  # Mount the src and dst rootfs.
+  local src_root_fs_dir="/tmp/m"
+  local image_dir=$(dirname "${src_image}")
+  local image_name=$(basename "${src_image}")
+  ${SCRIPTS_DIR}/mount_gpt_image.sh -f "${image_dir}" -i "${image_name}"

[gauravsh, 2010/12/22 22:55:27]

you can use mount_image_partition from
vboot_reference/scripts/image_signing/common.sh here.

It does tracked mounts, so you won't have to worry about unmounting it
explicitly (unless really needed).

[thieule, 2010/12/23 19:26:28]

Done.

+  add_cleanup_action "${SCRIPTS_DIR}/mount_gpt_image.sh -u"
+
+  local dst_root_fs_dir=$(mktemp -d "/tmp/align_root_fs_dest_mount_dir.XXXX")
+  add_cleanup_action "sudo rm -rf \"${dst_root_fs_dir}\""
+  sudo mount -o loop "${dst_root_fs}" "${dst_root_fs_dir}" -o loop
+  add_cleanup_action "sudo umount -d \"${dst_root_fs_dir}\""
+
+  # Temporarily make immutable files on the dst rootfs mutable.

[petkov, 2010/12/22 22:45:03]

do we have such files? do we care about mutable/immutable given that rootfs is
read-only?

[thieule, 2010/12/23 19:26:28]

There is one immutable file /boot/extlinux.sys that we have to temporarily make
mutable so we can overwrite it.  Even though the rootfs is mounted readonly, I
put the immutable flag back just to be clean.  I can remove the code that +i the
file if you like.

+  immutable_files=()
+  local dst_root_fs_dir_escaped=$(echo "${dst_root_fs_dir}" | \
+                                  sed 's/\//\\\//g')

[petkov, 2010/12/22 22:45:03]

use sed s,,, rather than sed s/// for readability?

[adlr, 2010/12/22 22:49:50]

maybe this is clearer:
s_/_\\/_g

since you can use any char for the / part of the s command in sed

[thieule, 2010/12/23 19:26:28]

Ack, missed this on the original code review.  Sosa mentioned it and it
completely slipped my mind.

Done.

+  local enum_files_cmd='sudo find "${dst_root_fs_dir}" -xdev -type f'
+  eval $enum_files_cmd | sed "s/${dst_root_fs_dir_escaped}//" | \

[adlr, 2010/12/22 22:49:50]

comment on what this command does?

[thieule, 2010/12/23 19:26:28]

I've removed this code.  It was a leftover from some older code where I was
implementing my own rsync instead of using rsync.  Should be easier to
understand now.

+    while read -r FILE ; do

[petkov, 2010/12/22 22:45:03]

while is FILE all caps?

[thieule, 2010/12/23 19:26:28]

Done.

+      immutable=$(sudo lsattr "${dst_root_fs_dir}$FILE" | cut -d' ' -f1 | \
+                  grep -q i ; echo $?)
+      if [ $immutable -eq 0 ] ; then
+        immutable_files=("${immutable_files[@]}" "${dst_root_fs_dir}$FILE")
+        sudo chattr -i "${dst_root_fs_dir}$FILE"
+      fi
+    done
+
+  # Copy files from the src rootfs over top of dst rootfs.
+  # Use the --inplace flag to preserve as much of the file system metadata
+  # as possible.
+  sudo rsync -v -a -H -A -x --force --inplace --numeric-ids --delete \
+      "${src_root_fs_dir}"/ "${dst_root_fs_dir}"
+
+  # Make immutable files immutable again.
+  for FILE in ${immutable_files[*]} ; do

[petkov, 2010/12/22 22:45:03]

why is FILE all caps?

[thieule, 2010/12/23 19:26:28]

Done.

+    sudo chattr +i "${dst_root_fs_dir}$FILE"
+  done
+
+  ${SCRIPTS_DIR}/mount_gpt_image.sh -u

[petkov, 2010/12/22 22:45:03]

you'd try to unmount this twice, I think -- once here and once on EXIT? you
don't need this unmount I guess.

[thieule, 2010/12/23 19:26:28]

I need these unmounts because I'll be replacing the rootfs in the image.  I've
added support for performing the cleanup action and removing from the list.

+  sudo umount -d "${dst_root_fs_dir}"
+}
+
+replace_root_fs() {

[gauravsh, 2010/12/22 22:55:27]

add comment about the arguments and what the function does

[thieule, 2010/12/23 19:26:28]

I've removed this function and using replace_image_partition() from common.sh
instead.

+  local root_fs=$1
+  local image=$2
+  local root_fs_offset=$(partoffset "${image}" 3)
+  local root_fs_count=$(partsize "${image}" 3)
+  dd if="${root_fs}" of="${image}" bs=512 conv=notrunc \

[adlr, 2010/12/22 22:49:50]

maybe do bs=$((2 * 1024 * 1024)) ? take a look at fast_dd in
platform/installer/chromeos-install . might be good to move that to
chromeos-common

[thieule, 2010/12/23 19:26:28]

Function removed.

+    seek=${root_fs_offset} count=${root_fs_count}
+}
+
+main() {
+  trap cleanup EXIT

[adlr, 2010/12/22 22:49:50]

many script trap on more than just exit, (i think) TERM and INT as well. Maybe
add those?

[gauravsh, 2010/12/22 22:55:27]

if you use vboot_reference's common.sh, this will conflict with its own cleanup
handler.

[thieule, 2010/12/23 19:26:28]

Done.

[thieule, 2010/12/23 19:26:28]

Done.

+
+  initialize
+
+  # Download the latest shipping image.
+  get_latest_shipping_version
+
+  local latest_shipping_image=$(mktemp "/tmp/align_rootfs_shipping_image.XXXX")
+  add_cleanup_action "sudo rm -f \"${latest_shipping_image}\""

[petkov, 2010/12/22 22:45:03]

why do this special add_cleanup_action rather than just:

trap "sudo rm -f \"${latest_shipping_image}\"" EXIT

then you don't need to run cleanup for EXIT and you don't need
add_cleanup_action

[thieule, 2010/12/23 19:26:28]

I have other clean up actions that may need to be executed other than just this
one.

+  info "Downloading image (${LATEST_SHIPPING_VERSION})"
+  get_shipping_image "${LATEST_SHIPPING_VERSION}" "${latest_shipping_image}"
+
+  # Make sure the two rootfs are the same size.
+  # If they are not, then there is nothing for us to do.
+  # Note: Exit with a zero code so we do not break the build workflow.
+  local shipping_root_fs_size=$(partsize "${latest_shipping_image}" 3)
+  local new_root_fs_size=$(partsize "${FLAGS_image}" 3)
+  if [ ${shipping_root_fs_size} -ne ${new_root_fs_size} ] ; then
+    info "The latest shipping rootfs and the new rootfs are not the same size."

[petkov, 2010/12/22 22:45:03]

warn instead?

[thieule, 2010/12/23 19:26:28]

Done.

+    exit 0
+  fi
+
+  # Extract the rootfs from the shipping image and use this as a template
+  # for the new image.
+  temp_root_fs=$(mktemp "/tmp/align_rootfs_temp_rootfs.XXXX")
+  add_cleanup_action "sudo rm -f \"${temp_root_fs}\""
+  info "Extracting rootfs from shipping image"
+  extract_root_fs "${latest_shipping_image}" "${temp_root_fs}"

[gauravsh, 2010/12/22 22:55:27]

extract_partition can be used here

[thieule, 2010/12/23 19:26:28]

Done.

+  enable_rw_mount "${temp_root_fs}"
+
+  # Copy the new rootfs over template rootfs to preserve as much of the

[adlr, 2010/12/22 22:49:50]

this is a super high level comment that could go at the top of the file. here
you could just say something like "perform the actual copy"

[thieule, 2010/12/23 19:26:28]

Done.

+  # metadata from the shipping rootfs as possible. This will ensure minimal
+  # disk shuffling when applying the auto-update.
+  info "Copying rootfs"
+  copy_root_fs "${FLAGS_image}" "${temp_root_fs}"
+
+  # Replace the rootfs in the new image with the aligned version.
+  info "Replacing rootfs"
+  replace_root_fs "${temp_root_fs}" "${FLAGS_image}"
+
+  # Make the image bootable again.
+  local image_dir=$(dirname "${FLAGS_image}")
+  local image_name=$(basename "${FLAGS_image}")
+  info "Make image bootable"
+  ${SCRIPTS_DIR}/bin/cros_make_image_bootable "${image_dir}" "${image_name}"
+
+  trap - EXIT
+
+  cleanup
+}
+
+main

[petkov, 2010/12/22 22:45:03]

main "$*"

[thieule, 2010/12/23 19:26:28]

Done.