On 6/2/23 12:54, Heinrich Schuchardt wrote: > On 6/2/23 18:43, Sean Anderson wrote: >> 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? > > All usages of fs_read() expect that actread returns the size of the complete > file, e.g. > > - sysboot_read_file() > - do_load() > - splash_load_fs() > > Other U-Boot file systems like FAT, EXT4 conform to this. > > Unfortunately struct fstype_info is not documented at all and the > documentation of fs_read() in include/fs.h does not point this out.
OK, then I agree that we should have a loop in read/write. And this should be documented :) --Sean