boot-directdisk: fix the support of vmdk

From: Joao Henrique Ferreira de Freitas <joaohf@gmail.com>

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 <joaohf@gmail.com>

[edit to change the usage of IMAGE_FSTYPE to IS_VMDK]

Signed-off-by: Saul Wold <sgw@linux.intel.com>
---
 meta/classes/boot-directdisk.bbclass | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

On Wed, 2014-01-22 at 22:50 -0800, Saul Wold wrote:
> +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?

p.

On Thu, Jan 23, 2014 at 4:50 AM, Saul Wold <sgw@linux.intel.com> wrote:
> From: Joao Henrique Ferreira de Freitas <joaohf@gmail.com>
>
> 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 <joaohf@gmail.com>
>
> [edit to change the usage of IMAGE_FSTYPE to IS_VMDK]
>
> Signed-off-by: Saul Wold <sgw@linux.intel.com>

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.

On Thu, 2014-01-23 at 07:04 +0000, Phil Blundell wrote:
> On Wed, 2014-01-22 at 22:50 -0800, Saul Wold wrote:
> > +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

On Thu, Jan 23, 2014 at 10:21 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> On Thu, 2014-01-23 at 07:04 +0000, Phil Blundell wrote:
>> On Wed, 2014-01-22 at 22:50 -0800, Saul Wold wrote:
>> > +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...

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...

Hi, Saul

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

Zhu Yanjun
On 01/23/2014 02:50 PM, Saul Wold wrote:
> From: Joao Henrique Ferreira de Freitas <joaohf@gmail.com>
>
> 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 <joaohf@gmail.com>
>
> [edit to change the usage of IMAGE_FSTYPE to IS_VMDK]
>
> Signed-off-by: Saul Wold <sgw@linux.intel.com>
> ---
>   meta/classes/boot-directdisk.bbclass | 18 ++++++++----------
>   1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/meta/classes/boot-directdisk.bbclass b/meta/classes/boot-directdisk.bbclass
> index 3277666..afab843 100644
> --- a/meta/classes/boot-directdisk.bbclass
> +++ b/meta/classes/boot-directdisk.bbclass
> @@ -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