Re: [PATCH 3/3] ios: report IOS_EPERM whenever appropriate

[Jose E. Marchesi]

> On 3/27/21 12:37 PM, Jose E. Marchesi wrote:
>> This patch makes the read/write IOS functions to return IOS_EPERM when
>> the IO space doesn't support reading (like in a write-only file) and
>> reading is needed.
>>
>> This includes reads of "completing bytes" that happens in certain
>> writes.
>>
>> Includes a few tests.
>>
>> 2021-03-27  Jose E. Marchesi  <jemarch@gnu.org>
>>
>>      * libpoke/ios.c (ios_read_uint): Return IOS_EPERM if the IO space
>>      is not readable.
>>      (ios_read_int): Likewise.
>>      (ios_read_string): Likewise.
>>      (IOS_GET_C_ERR_CHCK): Likewise.
>>      * testsuite/poke.map/maps-perm-1.pk: New test.
>>      * testsuite/poke.map/maps-perm-2.pk: Likewise.
>>      * testsuite/poke.map/maps-perm-3.pk: Likewise.
>>      * testsuite/poke.map/maps-perm-4.pk: Likewise.
>>      * testsuite/Makefile.am (EXTRA_DIST): Add new tests.
>> ---
>>   ChangeLog                         | 13 +++++++++++++
>>   libpoke/ios.c                     | 30 +++++++++++++++++++++++-------
>>   testsuite/Makefile.am             |  4 ++++
>>   testsuite/poke.map/maps-perm-1.pk |  6 ++++++
>>   testsuite/poke.map/maps-perm-2.pk |  6 ++++++
>>   testsuite/poke.map/maps-perm-3.pk |  6 ++++++
>>   testsuite/poke.map/maps-perm-4.pk | 10 ++++++++++
>>   7 files changed, 68 insertions(+), 7 deletions(-)
>>   create mode 100644 testsuite/poke.map/maps-perm-1.pk
>>   create mode 100644 testsuite/poke.map/maps-perm-2.pk
>>   create mode 100644 testsuite/poke.map/maps-perm-3.pk
>>   create mode 100644 testsuite/poke.map/maps-perm-4.pk
>>
>> diff --git a/ChangeLog b/ChangeLog
>> index d46d92fe..ed9f1726 100644
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,16 @@
>> +2021-03-27  Jose E. Marchesi  <jemarch@gnu.org>
>> +
>> +    * libpoke/ios.c (ios_read_uint): Return IOS_EPERM if the IO space
>> +    is not readable.
>> +    (ios_read_int): Likewise.
>> +    (ios_read_string): Likewise.
>> +    (IOS_GET_C_ERR_CHCK): Likewise.
>> +    * testsuite/poke.map/maps-perm-1.pk: New test.
>> +    * testsuite/poke.map/maps-perm-2.pk: Likewise.
>> +    * testsuite/poke.map/maps-perm-3.pk: Likewise.
>> +    * testsuite/poke.map/maps-perm-4.pk: Likewise.
>> +    * testsuite/Makefile.am (EXTRA_DIST): Add new tests.
>> +
>>   2021-03-27  Jose E. Marchesi  <jemarch@gnu.org>
>>      * libpoke/ios.h (IOS_EPERM): Define.
>> diff --git a/libpoke/ios.c b/libpoke/ios.c
>> index 75ea05de..68069486 100644
>> --- a/libpoke/ios.c
>> +++ b/libpoke/ios.c
>> @@ -32,13 +32,17 @@
>>   #include "ios.h"
>>   #include "ios-dev.h"
>>   -#define IOS_GET_C_ERR_CHCK(c, io, off)
>> \
>> -  {                                                                \
>> -    uint8_t ch;                                                        \
>> -    int ret = (io)->dev_if->pread ((io)->dev, &ch, 1, off);         \
>> -    if (ret == IOD_EOF)                                                \
>> -      return IOS_EIOFF;                                                \
>> -    (c) = ch;                                                        \
>> +#define IOS_GET_C_ERR_CHCK(c, io, off)                                  \
>> +  {                                                                     \
>> +    uint8_t ch;                                                         \
>> +    int ret;                                                            \
>> +    /* The IOS should be readable to get the completing byte.  */       \
>> +    if (!(io->dev_if->get_flags (io->dev) & IOS_F_READ))                \
>> +      return IOS_EPERM;                                                 \
>> +    ret = (io)->dev_if->pread ((io)->dev, &ch, 1, off);                 \
>> +    if (ret == IOD_EOF)                                                 \
>> +      return IOS_EIOFF;                                                 \
>> +    (c) = ch;                                                           \
>>     }
>>     #define IOS_PUT_C_ERR_CHCK(c, io, len, off)                \
>
> I dislike having this check in 'IOS_GET_C_ERR_CHCK'. That is called
> multiple times and it is about EOF or other errors that can change
> from one call to the other of the old get_char, what we call now
> pread.
>
> We can be sure that the flags won't change within a call to
> ios_write_int_common.Therefore, I believe this check should be moved
> to the beginning of ios_write_int_common. If that function is called,
> we now for sure that at least one of the first and last bytes to be
> written are not to be written 8 bits.

Oh, I didnt' realize that.  I thought ios_write_int_common _may_ write
without needing to read "completing bytes".

Perfect, will move the check to the beginning of the function.

> Now one more thing to discuss is the following: If we're writing past
> the end of a file, shall we pad it with 0s or shall we still return an 
> error? I vote for the former.

Yes me too.