Merge lp:~tom-gall/linaro-image-tools/linaro-media-create into lp:linaro-image-tools/11.11

[Tom Gall]

fixes lp:622979

Change correctly respects FAT_SIZE and replaces use of fdisk with sfdisk on MMC

[Loïc Minier (lool)]

Hey

 * ensure_command fdisk util-linux appears twice now
 * you replace FDISK=$(fdisk ...) with FDISK=$(sfdisk ...); might make sense to rename, but it's ok in both cases
 * duplicating the whole sudo sfdisk is a bit ugly; consider defining part_type=0xC or part_type=0xE, and using that in the here doc
 * I'm pretty sure that 0xC is supported, since the Ubuntu ARM folks release images using 0xC; however you might have to ensure that mkfs.vfat is called with -F32 to generate a FAT32 if you actually use 0xC (which means the partition is expected to contain a FAT32); instead of supporting FAT16 and FAT32 in the script, I'd rather force FAT32 everywhere

Thanks,

review: Disapprove

[Tom Gall (tom-gall)]

Hi,

Thanks for reviewing my proposed patch.

> Hey
>
> * ensure_command fdisk util-linux appears twice now

no it doesn't.

From the diff:
-ensure_command fdisk util-linux # threre is a fdisk binary provided by gnu-fdisk as well
+ensure_command sfdisk util-linux
+ensure_command fdisk util-linux

> * you replace FDISK=$(fdisk ...) with FDISK=$(sfdisk ...); might make sense
> to rename, but it's ok in both cases
> * duplicating the whole sudo sfdisk is a bit ugly; consider defining
> part_type=0xC or part_type=0xE, and using that in the here doc

If you know how to do the following construct correctly in bash I'm all ears:

sudo sfdisk -D -H $HEADS -S $SECTORS $CYLINDER_ARG $partdev << 'THEEND'
,9,$PARTITION_TABLE,*
,,,-
THEEND

I'm not sure evaluation of $PARTITION_TABLE is possible in that kind of construct ergo the if statement. You'll note on line 95 in the script the same kind of if statement is used with the exact same construct.

> * I'm pretty sure that 0xC is supported, since the Ubuntu ARM folks release
> images using 0xC; however you might have to ensure that mkfs.vfat is called
> with -F32 to generate a FAT32 if you actually use 0xC (which means the
> partition is expected to contain a FAT32); instead of supporting FAT16 and
> FAT32 in the script, I'd rather force FAT32 everywhere

The historical default for the script has been a FAT16 boot partitions. I don't see why advocating changing the default to FAT32. As I understand via Torez she had mentioned that not all boards supported FAT32. While this could be a broken firmware statement (and I've certainly seen that on other architectures), I don't believe changing the default to be wise. The essence of the change now allows boards to adjust the setting.

> Thanks,

Regards,
Tom

[Loïc Minier (lool)]

> no it doesn't.

This is the diff I received over email for mp+33843 (this request) had:
 ensure_command parted parted
-ensure_command fdisk util-linux # threre is a fdisk binary provided by
gnu-fdisk as well
+ensure_command fdisk util-linux
+ensure_command sfdisk util-linux
+ensure_command fdisk util-linux
 ensure_command wget wget

it clearly listed fdisk twice in the new version; this was apparently fixed in the mean time. What I see here now is alright though.

> If you know how to do the following construct correctly in bash I'm all ears

Sure, here you go:

cat << THEEND
,9,$PART_TYPE,*
,,,-
THEEND

note the lack of single quotes around 'THEEND'. EOF is a more common here doc delimiter, but either is fine.

> The historical default for the script has been a FAT16 boot partitions. I don't see why advocating changing the default to FAT32. As I understand via Torez she had mentioned that not all boards supported FAT32. While this could be a broken firmware statement (and I've certainly seen that on other architectures), I don't believe changing the default to be wise. The essence of the change now allows boards to adjust the setting.

The historical defaults for the script were a partition type of 0xC (means FAT32) and an actual filesystem of type FAT16 (mkfs.vfat without args selects FAT16/32 depending on the size of the partition, which was small enough that it would select FAT16); these were inconsistent.

I'd rather have us fix the inconsistency and find a common format which works with all boards than maintain two formats and code pathes.

[lp:~tom-gall/linaro-image-tools/linaro-media-create]

31. By Tom Gall on 2010-08-27

Fix to respect FAT_SIZE and not to use sfdisk for MMC targets - lp:NUMBER

32. By Tom Gall on 2010-08-27

Fix to add fstab entries for rootfs and boot

Unmerged revisions

=== modified file 'linaro-media-create'
--- linaro-media-create	2010-08-23 22:52:21 +0000
+++ linaro-media-create	2010-08-27 16:05:56 +0000
@@ -18,7 +18,8 @@
 
 ensure_command uuidgen uuidgen-runtime
 ensure_command parted parted
-ensure_command fdisk util-linux # threre is a fdisk binary provided by gnu-fdisk as well
+ensure_command sfdisk util-linux 
+ensure_command fdisk util-linux 
 ensure_command wget wget
 ensure_command mkfs.ext3 e2fsprogs
 ensure_command mkfs.ext4 e2fsprogs
@@ -129,10 +130,18 @@
 else
 partdev=${MMC}
 fi
-# Create a VFAT partition of 9 cylinders which is about 64M
+
+if [ "$FAT_SIZE" = "32" ]; then 
+	PARTITION_TYPE="0x0C"
+else
+	PARTITION_TYPE="0x0E"
+fi
+
+# Create a VFAT or FAT16 partition of 9 cylinders which is about 64M
 # and a linux partition of the rest
-sudo sfdisk -D -H $HEADS -S $SECTORS $CYLINDER_ARG $partdev << 'THEEND'
-,9,0x0C,*
+
+sudo sfdisk -D -H $HEADS -S $SECTORS $CYLINDER_ARG $partdev << THEEND
+,9,$PARTITION_TYPE,*
 ,,,-
 THEEND
 
@@ -279,14 +288,14 @@
 
 function check_mmc {
  DISK_NAME="Disk|Platte"
- FDISK=$(sudo fdisk -l | grep "[${DISK_NAME}] ${MMC}" | awk '{print $2}')
+ FDISK=$(sudo sfdisk -l | grep "[${DISK_NAME}] ${MMC}" | awk '{print $2}')
 
  if test "-$FDISK-" = "-$MMC:-"
  then
   echo ""
   echo "I see..."
-  echo "sudo fdisk -l:"
-  sudo fdisk -l | grep "[${DISK_NAME}] /dev/" --color=never
+  echo "sudo sfdisk -l:"
+  sudo sfdisk -l | grep "[${DISK_NAME}] /dev/" --color=never
   echo ""
   echo "mount:"
   mount | grep -v none | grep "/dev/" --color=never
@@ -298,8 +307,8 @@
   echo ""
   echo "Are you sure? I Don't see [${MMC}], here is what I do see..."
   echo ""
-  echo "sudo fdisk -l:"
-  sudo fdisk -l | grep "[${DISK_NAME}] /dev/" --color=never
+  echo "sudo sfdisk -l:"
+  sudo sfdisk -l | grep "[${DISK_NAME}] /dev/" --color=never
   echo ""
   echo "mount:"
   mount | grep -v none | grep "/dev/" --color=never