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

[Sean Anderson]

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.

--Sean