On 6/2/23 12:21, Heinrich Schuchardt wrote: > On 6/2/23 17:56, Sean Anderson wrote: >> On 6/2/23 04:48, Heinrich Schuchardt wrote: >>> >>> >>> On 5/18/23 17:17, Sean Anderson wrote: >>>> On 5/17/23 06:23, Heinrich Schuchardt wrote: >>>>> The return value of smh_flen() is written to size and not to ret. But ret >>>>> is checked. We can avoid calling smh_flen() by setting maxsize to LONG_MAX >>>>> if it is not set yet. >>>>> >>>>> Check input parameters. >>>>> >>>>> Fixes: f676b45151c3 ("fs: Add semihosting filesystem") >>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com> >>>>> --- >>>>> fs/semihostingfs.c | 14 +++++--------- >>>>> 1 file changed, 5 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/fs/semihostingfs.c b/fs/semihostingfs.c >>>>> index 96eb3349a2..8a7d4da884 100644 >>>>> --- a/fs/semihostingfs.c >>>>> +++ b/fs/semihostingfs.c >>>>> @@ -25,6 +25,9 @@ static int smh_fs_read_at(const char *filename, loff_t >>>>> pos, void *buffer, >>>>> { >>>>> long fd, size, ret; >>>>> + if (pos > LONG_MAX || maxsize > LONG_MAX) >>>> >>>> Should be ULONG_MAX. The type should really be ulong but isn't. >>>> >>>>> + return -EINVAL; >>>>> + >>>>> fd = smh_open(filename, MODE_READ | MODE_BINARY); >>>>> if (fd < 0) >>>>> return fd; >>>>> @@ -33,15 +36,8 @@ static int smh_fs_read_at(const char *filename, loff_t >>>>> pos, void *buffer, >>>>> smh_close(fd); >>>>> return ret; >>>>> } >>>>> - if (!maxsize) { >>>>> - size = smh_flen(fd); >>>>> - if (ret < 0) { >>>>> - smh_close(fd); >>>>> - return size; >>>>> - } >>>>> - >>>>> - maxsize = size; >>>>> - } >>>>> + if (!maxsize) >>>>> + maxsize = LONG_MAX; >>>> >>>> Same here. >>>> >>>>> size = smh_read(fd, buffer, maxsize); >>> >>> Thanks for reviewing. >>> >>> Just changing to ULONG_MAX will not work, because smh_read() returns errors >>> as negative numbers. >>> >>> We would need to change the interface of smh_read to: >>> >>> int smh_read(long fd, void *memp, unsigned long *len) >>> >>> Then we could return the error code and the number of bytes read separately. >> >> Sounds reasonable. >> >>> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdeveloper.arm.com%2fdocumentation%2fdui0471%2fm%2fwhat%2dis%2dsemihosting%2d%2fsys%2dread%2d%2d0x06%2d&umid=985b6a56-4e08-43b2-84c0-f256a70807de&auth=d807158c60b7d2502abde8a2fc01f40662980862-076ac538dd94fd766377877ad7a42d77df41830f >>> describes that SYS_READ may return less bytes than requested. This does not >>> signify that EOF has been reached but may simply reflect the behavior of >>> the underlying operating system (cf. 'man 2 read'). >>>> So shouldn't we loop here until all bytes are read? >> >> We return the number of bytes read. It's up to the caller to loop if they >> want. >> >> IMO for efficiency, the host should handle short reads to avoid multiple >> semihosting calls. > > Have a look at the QEMU code. host_read() calls read() once. So if the > underlying host file system does not read all bytes in the first invocation > (which is allowable), SYS_READ will not do so either.
Yes, I know that some hosts do not do this, but they should for efficiency. > U-Boot's do_load() calls _fs_read() only once. It expects the semihosting > filesystem to return the complete file. > > So there seems to be a gap in U-Boot's semihosting code. Isn't that what the fs_read's actread parameter is for? --Sean