| Message ID | 20160918132731.6293-1-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On Sun, Sep 18, 2016 at 03:27:31PM +0200, Marek Vasut wrote: > This patch broke booting of any fitImage-wrapped kernel images due > to replacement of ENOLINK with ENOENT without checking where the > ENOLINK return value is being tested for. Adjust the tests as well > to repair the breakage. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jonathan Gray <jsg@jsg.id.au> > Cc: Paul Burton <paul.burton@imgtec.com> > Cc: Tom Rini <trini@konsulko.com> Fixes: bac17b78dace ("image-fit: switch ENOLINK to ENOENT") Reviewed-by: Tom Rini <trini@konsulko.com>
Hi Marek, On 18/09/16 14:27, Marek Vasut wrote: > This patch broke booting of any fitImage-wrapped kernel images due > to replacement of ENOLINK with ENOENT without checking where the > ENOLINK return value is being tested for. Adjust the tests as well > to repair the breakage. It's not obvious from the commit message what "this patch" refers to - I think it would be useful to include the hash & subject of the broken commit, eg: Commit bac17b78dace ("image-fit: switch ENOLINK to ENOENT") broke... The (potential?) problem with the approach you took in this patch is that the return value from fit_get_node_from_config no longer differentiates between the ramdisk property of a FIT config being missing (-ENOLINK prior to bac17b78dace) & the configuration itself being missing (-ENOENT). Could that possibly lead to accepting invalid FIT images? If not then I imagine it's by some convoluted chance & that we'd probably be best to not rely on it. If using ENOLINK isn't an option then my suggestion would be that we change the config not found case to return -EINVAL & the property not found can keep returning -ENOENT. Semantically that would seem to make sense but it would mean checking all the callers, direct or indirect, of fit_get_node_from_config to see whether they rely upon -ENOENT for the config not found case. Thanks, Paul > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jonathan Gray <jsg@jsg.id.au> > Cc: Paul Burton <paul.burton@imgtec.com> > Cc: Tom Rini <trini@konsulko.com> > --- > common/image-fdt.c | 2 +- > common/image.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/common/image-fdt.c b/common/image-fdt.c > index d6ee225..3d23608 100644 > --- a/common/image-fdt.c > +++ b/common/image-fdt.c > @@ -285,7 +285,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, > fdt_noffset = fit_get_node_from_config(images, > FIT_FDT_PROP, > fdt_addr); > - if (fdt_noffset == -ENOLINK) > + if (fdt_noffset == -ENOENT) > return 0; > else if (fdt_noffset < 0) > return 1; > diff --git a/common/image.c b/common/image.c > index 7ad04ca..c8d9bc8 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -1078,7 +1078,7 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, > rd_addr = map_to_sysmem(images->fit_hdr_os); > rd_noffset = fit_get_node_from_config(images, > FIT_RAMDISK_PROP, rd_addr); > - if (rd_noffset == -ENOLINK) > + if (rd_noffset == -ENOENT) > return 0; > else if (rd_noffset < 0) > return 1; >
On 09/19/2016 10:29 AM, Paul Burton wrote: > Hi Marek, Hi, > On 18/09/16 14:27, Marek Vasut wrote: >> This patch broke booting of any fitImage-wrapped kernel images due >> to replacement of ENOLINK with ENOENT without checking where the >> ENOLINK return value is being tested for. Adjust the tests as well >> to repair the breakage. > > It's not obvious from the commit message what "this patch" refers to - I > think it would be useful to include the hash & subject of the broken > commit, eg: > > Commit bac17b78dace ("image-fit: switch ENOLINK to ENOENT") broke... > > The (potential?) problem with the approach you took in this patch is > that the return value from fit_get_node_from_config no longer > differentiates between the ramdisk property of a FIT config being > missing (-ENOLINK prior to bac17b78dace) & the configuration itself > being missing (-ENOENT). Could that possibly lead to accepting invalid > FIT images? If not then I imagine it's by some convoluted chance & that > we'd probably be best to not rely on it. Crap, that's a very valid point. > If using ENOLINK isn't an option then my suggestion would be that we > change the config not found case to return -EINVAL & the property not > found can keep returning -ENOENT. Semantically that would seem to make > sense but it would mean checking all the callers, direct or indirect, of > fit_get_node_from_config to see whether they rely upon -ENOENT for the > config not found case. Can you roll a patch for that ? Thanks! > Thanks, > Paul > >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Jonathan Gray <jsg@jsg.id.au> >> Cc: Paul Burton <paul.burton@imgtec.com> >> Cc: Tom Rini <trini@konsulko.com> >> --- >> common/image-fdt.c | 2 +- >> common/image.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/common/image-fdt.c b/common/image-fdt.c >> index d6ee225..3d23608 100644 >> --- a/common/image-fdt.c >> +++ b/common/image-fdt.c >> @@ -285,7 +285,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, >> fdt_noffset = fit_get_node_from_config(images, >> FIT_FDT_PROP, >> fdt_addr); >> - if (fdt_noffset == -ENOLINK) >> + if (fdt_noffset == -ENOENT) >> return 0; >> else if (fdt_noffset < 0) >> return 1; >> diff --git a/common/image.c b/common/image.c >> index 7ad04ca..c8d9bc8 100644 >> --- a/common/image.c >> +++ b/common/image.c >> @@ -1078,7 +1078,7 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, >> rd_addr = map_to_sysmem(images->fit_hdr_os); >> rd_noffset = fit_get_node_from_config(images, >> FIT_RAMDISK_PROP, rd_addr); >> - if (rd_noffset == -ENOLINK) >> + if (rd_noffset == -ENOENT) >> return 0; >> else if (rd_noffset < 0) >> return 1; >>
diff --git a/common/image-fdt.c b/common/image-fdt.c index d6ee225..3d23608 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -285,7 +285,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, fdt_noffset = fit_get_node_from_config(images, FIT_FDT_PROP, fdt_addr); - if (fdt_noffset == -ENOLINK) + if (fdt_noffset == -ENOENT) return 0; else if (fdt_noffset < 0) return 1; diff --git a/common/image.c b/common/image.c index 7ad04ca..c8d9bc8 100644 --- a/common/image.c +++ b/common/image.c @@ -1078,7 +1078,7 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, rd_addr = map_to_sysmem(images->fit_hdr_os); rd_noffset = fit_get_node_from_config(images, FIT_RAMDISK_PROP, rd_addr); - if (rd_noffset == -ENOLINK) + if (rd_noffset == -ENOENT) return 0; else if (rd_noffset < 0) return 1;
This patch broke booting of any fitImage-wrapped kernel images due to replacement of ENOLINK with ENOENT without checking where the ENOLINK return value is being tested for. Adjust the tests as well to repair the breakage. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Jonathan Gray <jsg@jsg.id.au> Cc: Paul Burton <paul.burton@imgtec.com> Cc: Tom Rini <trini@konsulko.com> --- common/image-fdt.c | 2 +- common/image.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)