Bug 587411 - livecd-iso-to-disk --format fails with SD/MMC partitions

[Frederick Grose]

Comments solicited on https://bugzilla.redhat.com/show_bug.cgi?id=587411.

         --Fred

[David Huff]

On 05/04/2010 10:26 PM, Frederick Grose wrote:
>  Comments solicited on
> https://bugzilla.redhat.com/show_bug.cgi?id=587411.
>
>            --Fred 
Frederick's patch fixes issues with SD cards that do not have a standard 
partitioning layout and requires part0 ie: /dev/mmcblk0

However the proposed patch requires users specify a
partition for the storage device or have --format option require a 
partition number.

Any comments on this?

Also Frederick can you please resend the patch to the list, that way I 
can apply it and retain all header info for the commit.


-D

[Alan Pevec]

On Mon, May 10, 2010 at 10:20 PM, David Huff <dhuff(a)redhat.com&gt; wrote:
>  Frederick's patch fixes issues with SD cards that do not have a
> standard
>  partitioning layout and requires part0 ie: /dev/mmcblk0 
issue is that partitions on mmc device use separator p i.e. 1st
partition is mmcblk0p1

>  However the proposed patch requires users specify a
>  partition for the storage device or have --format option require a
>  partition number. 
--format will create one, first partition, so it doesn't make sense to
specify the partition number

>  Any comments on this?
>
>  Also Frederick can you please resend the patch to the list, that way I
>  can apply it and retain all header info for the commit. 
patch in BZ also merged checkLVMActive() which is now committed

I don't like special case for mmc:
[[ "${path/*mmc*/yes}" = yes ]] && mmc=p

instead, why not simply detect partition separator here:

path=/dev/`basename $path`
partnum0=${path##$device}
partnum=${partnum0/#p/}
if [ "$partnum" = "$partnum0" ]; then
  separator=""
else
  separator="p"
fi
...
USBDEV=${device}${separator}${partnum:-1}
...

[Frederick Grose]

On Mon, May 10, 2010 at 6:27 PM, Alan Pevec <apevec(a)gmail.com&gt; wrote:

>  On Mon, May 10, 2010 at 10:20 PM, David Huff <dhuff(a)redhat.com&gt;
> wrote:
>  > Frederick's patch fixes issues with SD cards that do not have a standard
>  > partitioning layout and requires part0 ie: /dev/mmcblk0
>
>  issue is that partitions on mmc device use separator p i.e. 1st
>  partition is mmcblk0p1
>
>  > However the proposed patch requires users specify a
>  > partition for the storage device or have --format option require a
>  > partition number.
>
>  --format will create one, first partition, so it doesn't make sense to
>  specify the partition number
>  
Yes,  currently we don't support multiple partition devices.  That will be
another bug and patch upcoming.


>  > Any comments on this?
>  >
>  > Also Frederick can you please resend the patch to the list, that way I
>  > can apply it and retain all header info for the commit.
>
>  patch in BZ also merged checkLVMActive() which is now committed
>
>  I don't like special case for mmc:
>  [[ "${path/*mmc*/yes}" = yes ]] && mmc=p
>
>  instead, why not simply detect partition separator here:
>
>  path=/dev/`basename $path`
>  partnum0=${path##$device}
>  partnum=${partnum0/#p/}
>  if [ "$partnum" = "$partnum0" ]; then
>   separator=""
>  else
>   separator="p"
>  fi
>  ...
>  USBDEV=${device}${separator}${partnum:-1} 

A problem arises because users are allowed to --reset-mbr --format a
partition by simply supplying a device node such as /dev/sdb.  This means
the code must determine an appropriate partition name when one is not
supplied. So the suggested test fails for the case where "... --format
/dev/mmcblk0 ..." is supplied to livecd-iso-to-disk.

The path test works, but assumes that the separator is used, and apparently
some older systems did not use a 'p' separator.  So, better to just read
from /proc/partitions.

The following patch is proposed--but note: It has NOT been tested on EFI
code paths.

      --Fred


diff --git a/tools/livecd-iso-to-disk.sh b/tools/livecd-iso-to-disk.sh
index f4d0855..f92fe2b 100755
--- a/tools/livecd-iso-to-disk.sh
+++ b/tools/livecd-iso-to-disk.sh
@@ -47,21 +47,24 @@ getdisk() {
        return
     fi

-    p=$(udevadm info -q path -n $DEV)
-    if [ -e /sys/$p/device ]; then
- device=$(basename /sys/$p)
+    path=$(udevadm info -q path -n $DEV)
+    if [ -e /sys/$path/device ]; then
+    device=$(basename /sys/$path)
     else
- device=$(basename $(readlink -f /sys/$p/../))
+    device=$(basename $(readlink -f /sys/$path/../))
     fi
     if [ ! -e /sys/block/$device -o ! -e /dev/$device ]; then
- echo "Error finding block device of $DEV.  Aborting!"
- exitclean
+    echo "Error finding block device of $DEV.  Aborting!"
+    exitclean
     fi
-
-    device="/dev/$device"
+
     # FIXME: weird dev names could mess this up I guess
-    p=/dev/`basename $p`
-    partnum=${p##$device}
+    set $( awk '$4 ~ /'${device#/}'.*[0-9]$/ {print $4}' /proc/partitions
)
+    partition=$1
+    path=/dev/$(basename $path)
+    partnum=${path##$device}
+    [[ -z "$partnum" ]] && partnum=${partition##$device}
+    device="/dev/$device"
 }

 resetMBR() {
@@ -157,16 +160,17 @@ createGPTLayout() {
     echo "WARNING: THIS WILL DESTROY ANY DATA ON $device!!!"
     echo "Press Enter to continue or ctrl-c to abort"
     read
-    umount ${device}? &> /dev/null
+    umount ${device}* &> /dev/null
     /sbin/parted --script $device mklabel gpt
     partinfo=$(LC_ALL=C /sbin/parted --script -m $device "unit b print"
|grep ^$device:)
     size=$(echo $partinfo |cut -d : -f 2 |sed -e 's/B$//')
     /sbin/parted --script $device unit b mkpart '"EFI System
Partition"'
fat32 17408 $(($size - 17408)) set 1 boot on
-    USBDEV=${device}1
     # Sometimes automount can be _really_ annoying.
     echo "Waiting for devices to settle..."
     /sbin/udevadm settle
     sleep 5
+    partnum=$( awk '$4 ~ /'${device#/}'.*[0-9]$/ {print $4}'
/proc/partitions )
+    USBDEV=${device}${partnum}
     umount $USBDEV &> /dev/null
     /sbin/mkdosfs -n LIVE $USBDEV
     USBLABEL="UUID=$(/sbin/blkid -s UUID -o value $USBDEV)"
@@ -179,16 +183,17 @@ createMSDOSLayout() {
     echo "WARNING: THIS WILL DESTROY ANY DATA ON $device!!!"
     echo "Press Enter to continue or ctrl-c to abort"
     read
-    umount ${device}? &> /dev/null
+    umount ${device}* &> /dev/null
     /sbin/parted --script $device mklabel msdos
     partinfo=$(LC_ALL=C /sbin/parted --script -m $device "unit b print"
|grep ^$device:)
     size=$(echo $partinfo |cut -d : -f 2 |sed -e 's/B$//')
     /sbin/parted --script $device unit b mkpart primary fat32 17408
$(($size - 17408)) set 1 boot on
-    USBDEV=${device}1
     # Sometimes automount can be _really_ annoying.
     echo "Waiting for devices to settle..."
     /sbin/udevadm settle
     sleep 5
+    partnum=$( awk '$4 ~ /'${device#/}'.*[0-9]$/ {print $4}'
/proc/partitions )
+    USBDEV=${device}${partnum}
     umount $USBDEV &> /dev/null
     /sbin/mkdosfs -n LIVE $USBDEV
     USBLABEL="UUID=$(/sbin/blkid -s UUID -o value $USBDEV)"

[Frederick Grose]

Sorry for 'jumping the gun'.  Here's a more fully debugged version:

        --Fred

On Wed, May 12, 2010 at 4:35 PM, Frederick Grose <fgrose(a)gmail.com&gt; wrote:

>  On Mon, May 10, 2010 at 6:27 PM, Alan Pevec <apevec(a)gmail.com&gt;
> wrote: 
>   On Mon, May 10, 2010 at 10:20 PM, David Huff
> <dhuff(a)redhat.com&gt; wrote:
>
> > > Frederick's patch fixes issues with SD cards that do not have a standard
>
> > > partitioning layout and requires part0 ie: /dev/mmcblk0
>
> >  
>  issue is that partitions on mmc device use separator p i.e.
> 1st
>
> > partition is mmcblk0p1
>
> >  
>  > However the proposed patch requires users specify a
>
> > > partition for the storage device or have --format option require a
>
> > > partition number.
>
> >  
>  --format will create one, first partition, so it doesn't
> make sense to
>  specify the partition number
>  
Yes,  currently we don't support multiple partition devices.  That will be

>  another bug and patch upcoming. 
>  
> > Any comments on this?
>  
>>
>  
>  
>> > Also Frederick can you please resend the patch to
the list, that way I
>  
>> > can apply it and retain all header info for the
commit.
>  
>
>  
> patch in BZ also merged
checkLVMActive() which is now committed
>  
>
>  
> I don't like special case
for mmc:
>  
>> [[ "${path/*mmc*/yes}" = yes ]] &&
mmc=p
>  
>
>  
> instead, why not simply detect
partition separator here:
>  
>
>  
> path=/dev/`basename $path`
>  
>> partnum0=${path##$device}
>  
>> partnum=${partnum0/#p/}
>  
>> if [ "$partnum" = "$partnum0" ];
then
>  
>>  separator=""
>  
>> else
>  
>>  separator="p"
>  
>> fi
>  
>> ...
> USBDEV=${device}${separator}${partnum:-1}
>  
A problem arises because users are allowed to --reset-mbr --format a
partition by simply supplying a device node such as /dev/sdb.  This means
the code must determine an appropriate partition name when one is not
supplied. So the suggested test fails for the case where "... --format
/dev/mmcblk0 ..." is supplied to livecd-iso-to-disk.

>  
The path test works, but assumes that the separator is used,
and apparently
some older systems did not use a 'p' separator.  So, better to just read
from /proc/partitions.

>  
The following patch is proposed--but note: It has NOT been
tested on EFI
code paths.

>  
      --Fred


diff --git a/tools/livecd-iso-to-disk.sh b/tools/livecd-iso-to-disk.sh

>  index f4d0855..ad85839 100755 
>  --- a/tools/livecd-iso-to-disk.sh 
>  +++ b/tools/livecd-iso-to-disk.sh 
>  @@ -47,21 +47,24 @@ getdisk() { 
>          return 
>       fi 
>  
>  -    p=$(udevadm info -q path -n $DEV) 
>  -    if [ -e /sys/$p/device ]; then 
>  - device=$(basename /sys/$p) 
>  +    path=$(udevadm info -q path -n $DEV) 
>  +    if [ -e /sys/$path/device ]; then 
>  +    device=$(basename /sys/$path) 
>       else 
>  - device=$(basename $(readlink -f /sys/$p/../)) 
>  +    device=$(basename $(readlink -f /sys/$path/../)) 
>       fi 
>       if [ ! -e /sys/block/$device -o ! -e /dev/$device ]; then
>  - echo "Error finding block device of $DEV.  Aborting!"
>  - exitclean 
>  +    echo "Error finding block device of $DEV.  Aborting!"
>  +    exitclean 
>       fi 
>  - 
>  -    device="/dev/$device" 
>  + 
>       # FIXME: weird dev names could mess this up I guess 
>  -    p=/dev/`basename $p` 
>  -    partnum=${p##$device} 
>  +    set $( awk '$4 ~ /'$device'.*[0-9]$/ {print $4}'
> /proc/partitions ) 
>  +    partition=$1 
>  +    path=$(basename $path) 
>  +    partnum=${path##$device} 
>  +    [[ -z "$partnum" ]] &&
> partnum=${partition##$device} 
>  +    device="/dev/$device" 
>   } 
>  
>   resetMBR() { 
>  @@ -157,16 +160,18 @@ createGPTLayout() { 
>       echo "WARNING: THIS WILL DESTROY ANY DATA ON
> $device!!!" 
>       echo "Press Enter to continue or ctrl-c to abort"
>       read 
>  -    umount ${device}? &> /dev/null 
>  +    umount ${device}* &> /dev/null 
>       /sbin/parted --script $device mklabel gpt 
>       partinfo=$(LC_ALL=C /sbin/parted --script -m $device "unit
> b print" 
|grep ^$device:)

>       size=$(echo $partinfo |cut -d : -f 2 |sed -e 's/B$//')
>       /sbin/parted --script $device unit b mkpart '"EFI
> System Partition"' 
fat32 17408 $(($size - 17408)) set 1 boot on

>  -    USBDEV=${device}1 
>       # Sometimes automount can be _really_ annoying. 
>       echo "Waiting for devices to settle..." 
>       /sbin/udevadm settle 
>       sleep 5 
>  +    partition=$( awk '$4 ~ /'${device#/dev/}'.*[0-9]$/
> {print $4}' 
/proc/partitions )

>  +    partnum=${partition##${device#/dev/}} 
>  +    USBDEV=${device}${partnum} 
>       umount $USBDEV &> /dev/null 
>       /sbin/mkdosfs -n LIVE $USBDEV 
>       USBLABEL="UUID=$(/sbin/blkid -s UUID -o value
> $USBDEV)" 
>  @@ -179,16 +184,18 @@ createMSDOSLayout() { 
>       echo "WARNING: THIS WILL DESTROY ANY DATA ON
> $device!!!" 
>       echo "Press Enter to continue or ctrl-c to abort"
>       read 
>  -    umount ${device}? &> /dev/null 
>  +    umount ${device}* &> /dev/null 
>       /sbin/parted --script $device mklabel msdos 
>       partinfo=$(LC_ALL=C /sbin/parted --script -m $device "unit
> b print" 
|grep ^$device:)

>       size=$(echo $partinfo |cut -d : -f 2 |sed -e 's/B$//')
>       /sbin/parted --script $device unit b mkpart primary fat32 17408
$(($size - 17408)) set 1 boot on

>  -    USBDEV=${device}1 
>       # Sometimes automount can be _really_ annoying. 
>       echo "Waiting for devices to settle..." 
>       /sbin/udevadm settle 
>       sleep 5 
>  +    partition=$( awk '$4 ~ /'${device#/dev/}'.*[0-9]$/
> {print $4}' 
/proc/partitions )

>  +    partnum=${partition##${device#/dev/}} 
>  +    USBDEV=${device}${partnum} 
>       umount $USBDEV &> /dev/null 
>       /sbin/mkdosfs -n LIVE $USBDEV 
    
USBLABEL="UUID=$(/sbin/blkid -s UUID -o value $USBDEV)"