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

[Egeyar Bagcioglu]

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.

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.

Regards
Ege