On Sat, 20 Apr 2013 16:03:20 -0700 Simon Glass sjg@chromium.org wrote:
On Mon, Apr 1, 2013 at 5:13 PM, Kim Phillips kim.phillips@freescale.com wrote:
On Mon, 18 Mar 2013 16:51:20 -0700 Simon Glass sjg@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