boot-directdisk: fix the support of vmdk

PATCH In review Saul Wold 5 participants 6 messages Jan 23, 2014 – Jun 5, 2014

lore

Share & tools

lore

Saul Wold

author

2014-01-23 06:50 UTC #0 lore raw

PATCH 1/1 boot-directdisk: fix the support of vmdk

From: Joao Henrique Ferreira de Freitas <[email protected]>

Previous change (086ce22b88f5ef5f75a83119a32c8b3fdcfa296d) broke the creating of vmdk images. This protects shell expansion variables and let dd generate the image to be transformed to vmdk by image-vmdk.class.

Signed-off-by Joao Henrique Ferreira de Freitas <[email protected]>

[edit to change the usage of IMAGE_FSTYPE to IS_VMDK]

Signed-off-by Saul Wold <[email protected]>

---
 meta/classes/boot-directdisk.bbclass | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

meta/classes/boot-directdisk.bbclass +8 ~~-10~~

index 3277666..afab843 100644
@@ -61,6 +61,8 @@ DISK_SIGNATURE ?= "${DISK_SIGNATURE_GENERATED}"
 SYSLINUX_ROOT ?= "root=/dev/sda2"
 SYSLINUX_TIMEOUT ?= "10"
 
+IS_VMDK = '${@base_contains("IMAGE_FSTYPES", "vmdk", "true", "false", d)}'
+
 boot_direct_populate() {
 	dest=$1
 	install -d $dest

@@ -88,10 +90,10 @@ build_boot_dd() {
 		grubefi_hddimg_populate $HDDDIR
 	fi
 
-	if [ ${IMAGE_FSTYPE} = "vmdk" ]; then
-		if [ x${AUTO_SYSLINUXMENU} = x1 ] ; then
+	if [ "${IS_VMDK}" ]; then
+		if [ "x${AUTO_SYSLINUXMENU}" = "x1" ] ; then
 			install -m 0644 ${STAGING_DIR}/${MACHINE}/usr/share/syslinux/vesamenu.c32 ${HDDDIR}${SYSLINUXDIR}/vesamenu.c32
-			if [ x${SYSLINUX_SPLASH} != x ] ; then
+			if [ "x${SYSLINUX_SPLASH}" != "x" ] ; then
 				install -m 0644 ${SYSLINUX_SPLASH} ${HDDDIR}${SYSLINUXDIR}/splash.lss
 			fi
 		fi

@@ -129,9 +131,7 @@ build_boot_dd() {
 	parted $IMAGE unit B mkpart primary ext2 ${END2}B ${END3}B
 	parted $IMAGE set 1 boot on 
 
-	if [ ${IMAGE_FSTYPE} != "vmdk" ]; then
-		parted $IMAGE print
-	fi
+	parted $IMAGE print
 
 	awk "BEGIN { printf \"$(echo ${DISK_SIGNATURE} | fold -w 2 | tac | paste -sd '' | sed 's/\(..\)/\\x&/g')\" }" | \
 		dd of=$IMAGE bs=1 seek=440 conv=notrunc

@@ -141,10 +141,8 @@ build_boot_dd() {
 		dd if=${STAGING_DATADIR}/syslinux/mbr.bin of=$IMAGE conv=notrunc
 	fi
 
-	if [ ${IMAGE_FSTYPE} != "vmdk" ]; then
-		dd if=$HDDIMG of=$IMAGE conv=notrunc seek=1 bs=512
-		dd if=${ROOTFS} of=$IMAGE conv=notrunc seek=$OFFSET bs=512	
-	fi
+	dd if=$HDDIMG of=$IMAGE conv=notrunc seek=1 bs=512
+	dd if=${ROOTFS} of=$IMAGE conv=notrunc seek=$OFFSET bs=512
 
 	cd ${DEPLOY_DIR_IMAGE}
 	rm -f ${DEPLOY_DIR_IMAGE}/${IMAGE_LINK_NAME}.hdddirect

Signature · 1 line

1.8.3.1

Phil Blundell

2014-01-23 07:04 UTC #1 lore raw

meta/classes/boot-directdisk.bbclass DISK_SIGNATURE ?= "${DISK_SIGNATURE_GENERATED}"

Quoted 3 lines · Saul Wold

meta/classes/boot-directdisk.bbclass +3 -0

+IS_VMDK = '${@base_contains("IMAGE_FSTYPES", "vmdk", "true", "false", d)}'
+
+	if [ "${IS_VMDK}" ]; then

Does that really work? I can't quite see how it would, but I might be overlooking something. Presumably you did test this, right?

Richard Purdie

2014-01-23 12:21 UTC #2 lore raw

Quoted 7 lines · Phil Blundell

meta/classes/boot-directdisk.bbclass DISK_SIGNATURE ?= "${DISK_SIGNATURE_GENERATED}"
Quoted 3 lines · Saul Wold
meta/classes/boot-directdisk.bbclass +3 -0
+IS_VMDK = '${@base_contains("IMAGE_FSTYPES", "vmdk", "true", "false", d)}'
+
+	if [ "${IS_VMDK}" ]; then
Does that really work? I can't quite see how it would, but I might be overlooking something. Presumably you did test this, right?

I've merged this since I really would like to try and get the builds stabilised. I did fix the patch since it would always evaluate as true the way it was...

Cheers,

Richard

Otavio Salvador

2014-01-23 12:36 UTC #3 lore raw

Quoted 9 lines · Richard Purdie

Quoted 7 lines · Phil Blundell
meta/classes/boot-directdisk.bbclass DISK_SIGNATURE ?= "${DISK_SIGNATURE_GENERATED}"
Quoted 3 lines · Saul Wold
meta/classes/boot-directdisk.bbclass +3 -0
+IS_VMDK = '${@base_contains("IMAGE_FSTYPES", "vmdk", "true", "false", d)}'
+
+   if [ "${IS_VMDK}" ]; then
Does that really work? I can't quite see how it would, but I might be overlooking something. Presumably you did test this, right?

Quoted 3 lines

I've merged this since I really would like to try and get the builds stabilised. I did fix the patch since it would always evaluate as true the way it was...

What is the point in having:

+ if [ "${IS_VMDK}" = "true" ]; then

This is an indirection that buys nothing.

- if [ ${IMAGE_FSTYPE} != "vmdk" ]; then
- dd if=$HDDIMG of=$IMAGE conv=notrunc seek=1 bs=512
- dd if=${ROOTFS} of=$IMAGE conv=notrunc seek=$OFFSET bs=512

-       fi
+       dd if=$HDDIMG of=$IMAGE conv=notrunc seek=1 bs=512
+       dd if=${ROOTFS} of=$IMAGE conv=notrunc seek=$OFFSET bs=512

This changes the logic but commit log is not clear /what/ it fixes. I did valid comments in the patch and you didn't considered those.

I just wasted time reviewing it...

Signature · 3 lines

Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Otavio Salvador

2014-01-23 11:21 UTC #4 lore raw

In reply to Saul Wold

Quoted 11 lines · Saul Wold

From: Joao Henrique Ferreira de Freitas <[email protected]>
Previous change (086ce22b88f5ef5f75a83119a32c8b3fdcfa296d) broke the creating of vmdk images. This protects shell expansion variables and let dd generate the image to be transformed to vmdk by image-vmdk.class.
Signed-off-by Joao Henrique Ferreira de Freitas <[email protected]>
[edit to change the usage of IMAGE_FSTYPE to IS_VMDK]
Signed-off-by Saul Wold <[email protected]>

This is hard to review as it has two mixed changes.

It changes the IS_VMDK change and logical changes inside same patch. Please split it in two patches.

Shell checking for true and false is Bash specific.

$ if [ false ]; then echo "Is true"; else echo "Is false"; fi
Is true

So this does not work with Dash. Please use IMAGE_FSTYPE = "vmdk" for easy reading of the code.

Signature · 3 lines

Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

yzhu1

2014-06-05 05:11 UTC #5 lore raw

In reply to Saul Wold

Hi, Saul

Sorry. I do not understand. What does the patch fix? After this patch is applied, what is the difference?

Zhu Yanjun

Quoted 66 lines · Saul Wold

From: Joao Henrique Ferreira de Freitas <[email protected]>

Signed-off-by Joao Henrique Ferreira de Freitas <[email protected]>

[edit to change the usage of IMAGE_FSTYPE to IS_VMDK]

Signed-off-by Saul Wold <[email protected]>

---
  meta/classes/boot-directdisk.bbclass | 18 ++++++++----------
  1 file changed, 8 insertions(+), 10 deletions(-)

meta/classes/boot-directdisk.bbclass +8 ~~-10~~

index 3277666..afab843 100644
@@ -61,6 +61,8 @@ DISK_SIGNATURE ?= "${DISK_SIGNATURE_GENERATED}"
  SYSLINUX_ROOT ?= "root=/dev/sda2"
  SYSLINUX_TIMEOUT ?= "10"
 
+IS_VMDK = '${@base_contains("IMAGE_FSTYPES", "vmdk", "true", "false", d)}'
+
  boot_direct_populate() {
  	dest=$1
  	install -d $dest

@@ -88,10 +90,10 @@ build_boot_dd() {
  		grubefi_hddimg_populate $HDDDIR
  	fi
 
-	if [ ${IMAGE_FSTYPE} = "vmdk" ]; then
-		if [ x${AUTO_SYSLINUXMENU} = x1 ] ; then
+	if [ "${IS_VMDK}" ]; then
+		if [ "x${AUTO_SYSLINUXMENU}" = "x1" ] ; then
  			install -m 0644 ${STAGING_DIR}/${MACHINE}/usr/share/syslinux/vesamenu.c32 ${HDDDIR}${SYSLINUXDIR}/vesamenu.c32
-			if [ x${SYSLINUX_SPLASH} != x ] ; then
+			if [ "x${SYSLINUX_SPLASH}" != "x" ] ; then
  				install -m 0644 ${SYSLINUX_SPLASH} ${HDDDIR}${SYSLINUXDIR}/splash.lss
  			fi
  		fi

@@ -129,9 +131,7 @@ build_boot_dd() {
  	parted $IMAGE unit B mkpart primary ext2 ${END2}B ${END3}B
  	parted $IMAGE set 1 boot on
 
-	if [ ${IMAGE_FSTYPE} != "vmdk" ]; then
-		parted $IMAGE print
-	fi
+	parted $IMAGE print
 
  	awk "BEGIN { printf \"$(echo ${DISK_SIGNATURE} | fold -w 2 | tac | paste -sd '' | sed 's/\(..\)/\\x&/g')\" }" | \
  		dd of=$IMAGE bs=1 seek=440 conv=notrunc

@@ -141,10 +141,8 @@ build_boot_dd() {
  		dd if=${STAGING_DATADIR}/syslinux/mbr.bin of=$IMAGE conv=notrunc
  	fi
 
-	if [ ${IMAGE_FSTYPE} != "vmdk" ]; then
-		dd if=$HDDIMG of=$IMAGE conv=notrunc seek=1 bs=512
-		dd if=${ROOTFS} of=$IMAGE conv=notrunc seek=$OFFSET bs=512	
-	fi
+	dd if=$HDDIMG of=$IMAGE conv=notrunc seek=1 bs=512
+	dd if=${ROOTFS} of=$IMAGE conv=notrunc seek=$OFFSET bs=512
 
  	cd ${DEPLOY_DIR_IMAGE}
  	rm -f ${DEPLOY_DIR_IMAGE}/${IMAGE_LINK_NAME}.hdddirect