[PATCH] fs: semihosting: Use correct variable for error check

[Tom Rini]

On Fri, Oct 17, 2025 at 01:08:20PM -0400, Sean Anderson wrote:
> On 10/15/25 12:28, Tom Rini wrote:
> > On Thu, Oct 02, 2025 at 10:36:07AM -0400, Sean Anderson wrote:
> >> On 10/2/25 05:52, Heinrich Schuchardt wrote:
> >> > On 10/2/25 11:39, Andrew Goodbody wrote:
> >> >> After calling a function that can return an error, the test to detect
> >> >> that error should use the return value not a different variable. Fix it.
> >> >>
> >> >> This issue was found by Smatch.
> >> >>
> >> > 
> >> > Fixes: f676b45151c3 ("fs: Add semihosting filesystem")
> >> > 
> >> >> Signed-off-by: Andrew Goodbody <andrew.goodbody at linaro.org>
> >> >> ---
> >> >>   fs/semihostingfs.c | 2 +-
> >> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/fs/semihostingfs.c b/fs/semihostingfs.c
> >> >> index 77e39ca407e4d240a1fd573497c5b6b908816454..9d7a136b9ba9b035545b34b31df58e2d65de7db9 100644
> >> >> --- a/fs/semihostingfs.c
> >> >> +++ b/fs/semihostingfs.c
> >> >> @@ -35,7 +35,7 @@ static int smh_fs_read_at(const char *filename, loff_t pos, void *buffer,
> >> >>       }
> >> >>       if (!maxsize) {
> >> >>           size = smh_flen(fd);
> >> >> -        if (ret < 0) {
> >> >> +        if (size < 0) {
> >> > 
> >> > The ARM specification (https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdeveloper.arm.com%2fdocumentation%2fdui0203%2fj%2fsemihosting%2fsemihosting%2doperations%2fsys%2dflen%2d%2d0x0c%2d&umid=f17ab8fc-2d9d-4cce-91e2-e7e66f117613&rct=1759398729&auth=d807158c60b7d2502abde8a2fc01f40662980862-e0799d2d4d61f93cae033d54733c8fa50ef84918) has:
> >> > 
> >> > SYS_FLEN (0x0C)
> >> > 
> >> > Returns the length of a specified file.
> >> > On exit, R0 contains:
> >> >     the current length of the file object, if the call is successful
> >> >     -1 if an error occurs.
> >> > 
> >> > Please, consider that the file length on 32bit systems may exceed 2^31 You must not consider this as an error.
> >> > 
> >> > %s/if (size < 0)/if (size == -1L)/
> >> 
> >> This cannot occur because on a 32-bit system SYS_FLEN only returns
> >> 32-bits of information. The host must detect files that exceed 2 GiB in
> >> size and return -1 (simulating EOVERFLOW).
> > 
> > OK, but this is for 64-bit systems too, isn't it?
> > 
> 
> Sorry, I worded that poorly. What I mean is that the spec says
> 
> | On exit, R0 contains:
> |     the current length of the file object, if the call is successful
> |     -1 if an error occurs.
> 
> which implies that R0 is a signed integer (otherwise they would have
> specified 0xffffffff or 0xffffffffffffffff as the error value). So
> negative values (other than -1) indicate a negative file size (or are
> unspecified).
> 
> However, both OpenOCD and QEMU just take the return from stat(2) and
> copy it into R0. With a 32-bit host there is no problem since stat(2)
> will itself fail for files over 2 GiB. With a 32-bit target, this
> produces erroneous values for files larger than 4 GiB. Files sizes
> between 2 and 4 GiB could be recovered, but I don't think we should do
> that. On a 64-bit target, there is again no ambiguity (at least until 8
> EiB file sizes).
> 
> I don't think we should try to interpret negative values because it's
> ambiguous in the spec, the hosts known to return such values do so
> because of flaws in their implementation, and large files themselves
> have dubious merit.

Thanks for explaining more, it helps.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20251017/015b5396/attachment.sig>