[U-Boot] [PATCH 00/14] mkimage: Tidy up error handling

[Stefano Babic]

Hi Tom, Simon,

On 01/07/2016 18:45, Tom Rini wrote:
> On Fri, Jul 01, 2016 at 09:15:11AM -0700, Simon Glass wrote:
>> Hi,
>>
>> On 1 July 2016 at 08:48, Tom Rini <trini at konsulko.com> wrote:
>>> On Fri, Jul 01, 2016 at 10:44:04AM +0200, Stefano Babic wrote:
>>>> Hi Simon,
>>>>
>>>> On 30/06/2016 18:52, Simon Glass wrote:
>>>>> There are a few problems when mkimage is provided with invalid arguments.
>>>>> In one case it crashes. When an invalid image type it is provided it lists
>>>>> the valid types, but this is not implemented for compression, architecture
>>>>> or OS.
>>>>>
>>>>
>>>> There is another issue related to mkimage. It looks like it is broken
>>>> since a lot of time, but it appears it was not noted.
>>>>
>>>> mkimage -l is broken. Testing with i.MX images (imximage), it does not
>>>> show the header, but it reports the output as it as a "gpimage".
>>>>
>>>> In fact:
>>>>
>>>> ./tools/mkimage -l test.imx
>>>> GP Header: Size d1002040 LoadAddr 8017
>>>>
>>>> It should be:
>>>>
>>>> ./tools/mkimage -l test.imx
>>>> Image Type:   Freescale IMX Boot Image
>>>> Image Ver:    2 (i.MX53/6/7 compatible)
>>>> Data Size:    331776 Bytes = 324.00 kB = 0.32 MB
>>>> Load Address: 177ff420
>>>> Entry Point:  17800000
>>>>
>>>> The reason is due to the format of the gpimage itself. It has no magic
>>>> number, and checking for its correctness means in gph_verify_header()
>>>> just check that size and address are not NULL. That let think that the
>>>> image is a gpimage, it is not. gpimage simply uses the image and does
>>>> not let to check for further image headers that are put int the (sorted)
>>>> list with the U_BOOT_IMAGE_TYPE. Image types that are in the list
>>>> *before* gpimage are correctly recognized and printed, the following (as
>>>> imximage) not.
>>>>
>>>> A quick fix (maybe just for the release ?) should be to let gpimage (but
>>>> I do not know if there is another image type that misbehaves as it does)
>>>> as the last one in the list: if all detections fail, the last one
>>>> without any possibility for detection (gpimage) runs. The following
>>>> patch works for me:
>>>>
>>>> diff --git a/tools/Makefile b/tools/Makefile
>>>> index 63355aa..f72294a 100644
>>>> --- a/tools/Makefile
>>>> +++ b/tools/Makefile
>>>> @@ -76,8 +76,6 @@ dumpimage-mkimage-objs := aisimage.o \
>>>>                       lib/fdtdec.o \
>>>>                       fit_common.o \
>>>>                       fit_image.o \
>>>> -                     gpimage.o \
>>>> -                     gpimage-common.o \
>>>>                       common/image-fit.o \
>>>>                       image-host.o \
>>>>                       common/image.o \
>>>> @@ -100,6 +98,8 @@ dumpimage-mkimage-objs := aisimage.o \
>>>>                       zynqimage.o \
>>>>                       zynqmpimage.o \
>>>>                       $(LIBFDT_OBJS) \
>>>> +                     gpimage.o \
>>>> +                     gpimage-common.o \
>>>>                       $(RSA_OBJS-y)
>>>>
>>>>  dumpimage-objs := $(dumpimage-mkimage-objs) dumpimage.o
>>>>
>>>> What do you think ? If this could be a solution for release, I send a
>>>> formal patch.
>>>
>>> Ug, I think we need what you're saying at least for release.  I'm
>>> kicking off a big bisect now to see just when this last worked right.
>>
>> That patch seems reasonable to me for now. My series is intended for
>> the next release.
>>
>> Perhaps verify_header() should return a value meaning 'possibly'?
> 
> So, this was broken by:
> commit 0ca6691c2ec20ff264d882854c339795579991f5
> Author: Guilherme Maciel Ferreira <guilherme.maciel.ferreira at gmail.com>
> Date:   Thu Jan 15 02:48:05 2015 -0200
> 
>     imagetool: move common code to imagetool module
> 

Yes, I had bisected myself and I get this commit - but the patch is not
the readon of the breakage.

> And yes, perhaps some way to say to say "maybe" is enough.

Right, but there is a side effect. Even if a "maybe" is returned (or
let's say, a "I can't check"), the effect is that "GP Header: Size XXXX"
is still printed, because for the gpimage this *could* be a correct image.

>  Or maybe we
> just need to allow some to say that they can't verify?  I think we're thinking
> along the same lines.

Regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================