[PATCH 1/1] fs: fix smh_fs_read_at()

[Sean Anderson]

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.schuchardt at 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