Re: [PATCH 3/5] image_types.bbclass: fix EXTRA_IMAGECMD_sum.jffs2

[Richard Purdie <richard.purdie@linuxfoundation.org>]

On Tue, 2013-10-29 at 12:17 +0100, Andrea Adami wrote:
> On Tue, Oct 29, 2013 at 12:01 PM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > On Mon, 2013-10-21 at 00:34 +0200, Andrea Adami wrote:
> >> When overriding EXTRA_IMAGE_CMD_jffs2 = "--pad=foo ..."
> >> we are passing a malformed option to sumtool:
> >>
> >> sumtool: option '--pad' doesn't allow an argument
> >>
> >> Fix this by declaring a separate variable for the purpose.
> >>
> >> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
> >> ---
> >>  meta/classes/image_types.bbclass | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/meta/classes/image_types.bbclass b/meta/classes/image_types.bbclass
> >> index b8779e0..21391e8 100644
> >> --- a/meta/classes/image_types.bbclass
> >> +++ b/meta/classes/image_types.bbclass
> >> @@ -141,8 +141,8 @@ XZ_INTEGRITY_CHECK ?= "crc32"
> >>  XZ_THREADS ?= "-T 0"
> >>
> >>  IMAGE_CMD_jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD}"
> >> -IMAGE_CMD_sum.jffs2 = "${IMAGE_CMD_jffs2} && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 \
> >> -     -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
> >> +IMAGE_CMD_sum.jffs2 = "mkfs.jffs2 --root=${IMAGE_ROOTFS} --faketime --output=${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 ${EXTRA_IMAGECMD_jffs2} \
> >> +     && sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}"
> >>
> >>  IMAGE_CMD_cramfs = "mkfs.cramfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cramfs ${EXTRA_IMAGECMD}"
> >>
> >> @@ -212,6 +212,7 @@ inherit siteinfo
> >>  JFFS2_ENDIANNESS ?= "${@base_conditional('SITEINFO_ENDIANNESS', 'le', '-l', '-b', d)}"
> >>  JFFS2_ERASEBLOCK ?= "0x40000"
> >>  EXTRA_IMAGECMD_jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
> >> +EXTRA_IMAGECMD_sum.jffs2 ?= "--pad ${JFFS2_ENDIANNESS} --eraseblock=${JFFS2_ERASEBLOCK} --no-cleanmarkers"
> >>
> >>  # Change these if you want default mkfs behavior (i.e. create minimal inode number)
> >>  EXTRA_IMAGECMD_ext2 ?= "-i 8192"
> >
> > This patch is very confused. You say sumtool doesn't take a --pad
> > option, yet "EXTRA_IMAGECMD_sum.jffs" which is presumably used with
> > sumtool does have the option.
> >
> 
> After the commits you've done the creation of sum.jffs2 images is
> broken on collie.
> The error message is described clearly in the patch:
> 
> sumtool: option '--pad' doesn't allow an argument
> 
> Why that? Because collie needs to customize the padding.
> EXTRA_IMAGECMD_jffs2 = "--pad=14680064 -l -e ${JFFS2_ERASEBLOCK}"
> 
> This --pad=14680064 is then passed to IMAGE_CMD_sum.jffs2 and we get
> build error.
> 
> That's why we need to redefine both IMAGE_CMD_sum.jffs2 and
> EXTRA_IMAGECMD_sum.jffs2 so to pass --pad=XY only to mkfs.jffs2 and
> not to the sumtool part of IMAGE_CMD_sum.jffs2.
> 
> ---
> By the way this sum.jffs2 imagetype is used only in meta-handheld afaik.
> It would be also ok if you'd remove IMAGE_CMD_sum.jffs2 so we can
> append the whole stuff in a customized EXTRA_IMAGECMD_jffs2.

I understand your problem however:

a) Copy and pasting the entire IMAGE_CMD_jffs2 command into the second
one is rather ugly.
b) You still end up passing "--pad ${JFFS2_ENDIANNESS} --eraseblock=
${JFFS2_ERASEBLOCK} --no-cleanmarkers" as parameters to sumtool,
including a --pad option with no argument. Why/how?

You call:

sumtool -i ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.jffs2 -o ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.sum.jffs2 ${EXTRA_IMAGECMD}

and EXTRA_IMAGECMD is set to EXTRA_IMAGECMD_sum.jffs2 due to the use of
overrides in the image generation code.

So whilst collie is broken, I do not think this patch makes readable
code and I think we need to find something better.

The thing I'm confused on is whether there should be any --pad option to
sumtool or not.

Cheers,

Richard