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

login
register
mail settings
Submitter Andrea Adami
Date Oct. 20, 2013, 10:34 p.m.
Message ID <5739f47649bfb50369107ca603b747b4201c70b0.1382307169.git.andrea.adami@gmail.com>
Download mbox | patch
Permalink /patch/60281/
State Superseded, archived
Headers show

Comments

Andrea Adami - Oct. 20, 2013, 10:34 p.m.
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(-)
Andrea Adami - Oct. 29, 2013, 10:26 a.m.
BUMP

I've seen the first two bugfixes have been applied and now it is
possible to override EXTRA_IMAGECMD_jffs2.

This patch fixes the case of sum.jffs2 when a padding value is
specified because sumtool won't swallow it.

Thx

Andrea


On Mon, Oct 21, 2013 at 12:34 AM, Andrea Adami <andrea.adami@gmail.com> 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"
> --
> 1.8.1.5
>
Richard Purdie - Oct. 29, 2013, 11:01 a.m.
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.

We need to make this clearer and I don't think this patch does that, I'm
not even sure it works...

Cheers,

Richard
Andrea Adami - Oct. 29, 2013, 11:17 a.m.
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.

Interestingly the sum.jffs2 ican not be padded to a desired size and
that could cause some issues flashing over older jffs partitions
(happened on collie flashing on top of previous jffs format).

In the case of modern devices with NAND there is a clear benefit.

Regards

Andrea

> We need to make this clearer and I don't think this patch does that, I'm
> not even sure it works...
>
> Cheers,
>
> Richard
>
Richard Purdie - Oct. 29, 2013, 11:45 a.m.
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

Patch

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"