Message ID | 1390459814-15764-1-git-send-email-sgw@linux.intel.com |
---|---|
State | New |
Headers | show |
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
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