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
