[U-Boot] [PATCH v2 0/45] Verified boot implementation based on FIT

[Kim Phillips]

On Sat, 20 Apr 2013 16:03:20 -0700
Simon Glass <sjg at chromium.org> wrote:

> On Mon, Apr 1, 2013 at 5:13 PM, Kim Phillips <kim.phillips at freescale.com> wrote:
> > On Mon, 18 Mar 2013 16:51:20 -0700
> > Simon Glass <sjg at chromium.org> wrote:
> >
> >> I have received a number of off-list comments - please do copy the list when
> >> replying so that everyone can see your comments.
> >
> > I don't have time to fully review 45 patches, let alone the subject
> > matter (e.g., no support for RSA in h/w, eh? ;), but I did notice
> > some things that bugged me:
> >
> > Re: "[PATCH v2 04/45] libfdt: Add fdt_next_subnode() to permit easy subnode iteration":
> >
> > - Where's our libfdt maintainer?  libfdt patches should be submitted
> > to the dtc project first.  It appears "[PATCH v2 40/45] libfdt: Add
> > fdt_find_regions()" also suffers from this problem.
> 
> The fdt_next_subnode() is a pretty trivial change. I will submit it to
> the dtc project.

Ideally we'd apply the patch directly from how it was applied to the
upstream dtc project.

> The fdt_find_regions() was submitted to dtc as part of the dtc fdtgrep
> series. I modified the form of it in response to review feedback and
> that is ongoing.

ok.

> > - can we improve on the readability of that for loop - the improved
> > one the patch gives us is this:
> >
> >> for (ndepth = 0,
> >>               noffset = fdt_next_subnode(fit, image_noffset, &ndepth);
> >>               noffset >= 0;
> >>               noffset = fdt_next_subnode(fit, noffset, &ndepth)) {
> >
> > whereas perhaps something like this:
> >
> > ndepth = 0;
> > noffset = fdt_next_subnode(fit, image_noffset, &ndepth);
> > while (noffset >= 0) {
> >         // body
> >         noffset = fdt_next_subnode(fit, noffset, &ndepth);
> > }
> >
> > would be much easier to quickly visually parse?
> 
> This suffers from the problem that a 'continue' will not work as
> expected.

It's not like that can't be fixed - either by manually assigning
noffset before it, or by using a goto.

> I think the existing for() loop is a little better overall -
> everything is in one place.

it's ugly and comparatively unreadable, esp. given its alignment and
the ndepth initialization...

> > this would also affect:
> >
> > Re: "[PATCH v2 12/45] image: Move hash checking into its own function":
> > - which also adds lines like these:
> >
> >> +             *err_msgp = " error!\nCan't get hash algo "
> >> +                             "property";
> >
> > upon which a subsequent patch ("[PATCH v2 13/45] image: Move error!
> > string to common place", if I'm not mistaken) corrects.  Are you
> > intentionally trying to make people review the same code twice??
> 
> No, what is happening here is that the first patch moves the code into
> a function, and the second patch removes the 'error' part of the
> strings, not just from that moved code but from everywhere. I find
> that doing two things at once is harder to review.
> 
> I am certainly not added 'error' to the strings in one patch and
> taking it away in the next :-)

it sounds like the 'error' string removal should appear earlier in
the patchseries.

> > Re: "[PATCH v2 07/45] image: Split FIT code into new image-fit.c"
> > - after the code is split, it appears the rest of the patchseries
> > works on improving image-fit.c instead of updating equivalent code
> > still existing in image.c, IIRC, "[PATCH v2 13/45] image: Move error!
> > string to common place"
> > - I also found it odd that git format-patch -M on this doesn't
> > produce a shorter patch with "% equivalent" statistics: what else
> > changed?
> 
> Are you suggesting that more patches should affect image.c? If so,
> please let me know what should specifically you want done and I will
> take a look. Or perhaps we can improve it once the dust clears.

as above, I believe the answer is to make the common changes (to
both image.c and image-fit.c) earlier in the patchseries.

Thanks,

Kim