Re: [PATCH] [IOS] Avoid spurious EOF when writing partial bytes

[Georgiy Tugai]

Hi all,

I feel that there are a few different options here;

a) Don't support write-only IOS at all, since poke may need to read.

   This is consistent, but boring :)

b) Support write-only IOS, with the assumption that the backing store
   does not change out from under poke, in which case it is safe to
   cache just-written data in poke, thus poke knows what it "would
   have" read.

   This is consistent... up until the user encounters an IOS with
   other writers and principle of least surprise is violated.

c) [your proposal] Support write-only IOS, but anything that would
   need to read fails with an exception.

   This is something of a consistency breakage; everywhere else, poke
   appears to ignore the common limitations of byte-level writing,
   etc.

   Using an EOF exception would be quite confusing for the user as
   well; if this is implemented, please use a more accurate exception
   (maybe a new one needs to be added)

   With this idea, if the user wishes to use a write-only IOS in a
   fully general manner, they must use another IOS as temporary
   storage.

d) Support write-only IOS, with some explicit flag/state enabling
   caching as per option B.

   I believe this would provide a level of consistency for poke users,
   while maintaining least-surprise. If the user is using write-only
   IOS, which is something of an advanced feature, they can check the
   manual and/or the exception message to discover the caching feature :)

e) Implement transactional I/O in poke.

   This is the "fun" option, which may be over-ambitious. Write-only
   IOS and read-write IOS which may have other concurrent writers
   present a common challenge. Thus, it may be interesting to
   introduce a flag/state where all write operations are queued up in
   an implicit memory IOS (or an explicit arbitrary IOS?) and then
   committed or rolled back by the user.

Regards,
Georgiy

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, March 27, 2021 10:07 AM, Jose E. Marchesi <jemarch@gnu.org> wrote:

>
>
> I forgot to mention a corollary of this bug.
>
> In poke sometimes writing requires reading, like for example when
> writing weird integers or unaligned integers, i.e. when working with
> partial bytes, which should be "completed" with their current value in
> the IO space.
>
> Now, we support write-only IO spaces.
>
> Without this patch, this raises EOF:
>
> (poke) var fd = open ("lalala", IOS_M_WRONLY | IOS_F_CREATE)
> (poke) int @ 0#B = 0xffffffff
> -----> at this point the IO space has 4 0xff bytes
> (poke) uint<3> @ 0#B = 2
> unhandled EOF exception
>
> With this patch, the EOF exception is gone, but since the completing
> byte cannot be read it is assumed to be 0, and therefore the resulting
> data is not the right one:
>
> (poke) var fd = open ("lalala", IOS_M_WRONLY | IOS_F_CREATE)
> (poke) int @ 0#B = 0xffffffff
> ------> With this patch, the write above will use 0 to complete the
>
>           first byte of the file, instead of 0xff!
>
>
> (poke) uint<3> @ 0#B = 2
> (poke) close (fd)
> (poke) .file lalala
> (poke) dump
> 76543210 0011 2233 4455 6677 8899 aabb ccdd eeff 0123456789ABCDEF
> 00000000: 40ff ffff @...
>
> So I propose the following fix:
>
> -   If reading a completing byte is needed in a write, and the IO space
>     doesn't allow reading, the write operation should raise an EOF
>     exception.
>
>     WDYT?
>
>
> > Hi Egeyar, all.
> > While working on other bugs we found an issue with the IOS. We are
> > proposing the following patch.
> > Generally speaking, an IO space can be made to grow by mapping (writing)
> > data past the end.
> > Lets create an empty file:
> > (poke) .file/c lalala
> > (poke) dump
> > 76543210 0011 2233 4455 6677 8899 aabb ccdd eeff 0123456789ABCDEF
> > 00000000:
> > (poke) iosize/#B
> > 0UL
> > Then we write past the end:
> > (poke) byte @ 3#B = 0xff
> > (poke) dump
> > 76543210 0011 2233 4455 6677 8899 aabb ccdd eeff 0123456789ABCDEF
> > 00000000: 0000 00ff ....
> > (poke) iosize/#B
> > 4UL
> > This is the expected behavior.
> > However, consider what happens when you write weird integers past the
> > end of the IO space, like in:
> > (poke) uint<3> @ iosize = 2
> > In this case, the IO subsystem has to read a byte in order to complete
> > the contents of the partially written byte. Currently this results in
> > an EOF exception being raised by ios_write_u?int. No good.
> > In the patch attached:
> >
> > -   We change the macro IOS_GET_C_ERR_CHCK in order to return a zero
> >     completing byte instead of returning IOS_EIOFF. This relies in the
> >     fact the macro is only used in ios_write_int_common, not in
> >     ios_read_int_common.
> >
> > -   We add a new test for this.
> >
> > -   We rename the test maps-strings-diag-4.pk to maps-strings-4.pk, and
> >     adjust it accordingly.
> >
> >
> > The last point is due to the fact that we are changing semantics here.
> > Up to now, trying to write a value partially past the end of the IO
> > space resulted in an error (EIOFF, invalid offset):
> >
> >       |     IO space     |
> >                       < value >
> >
> >
> > Now, this just grows the IO space, for any kind of value: integers,
> > strings, etc.
> > Does this look good to you?
> > commit 867d7ff1641264b560f0b0fad6ab0931cf8ab204
> > Author: Jose E. Marchesi jose.marchesi@oracle.com
> > Date: Sat Mar 27 09:53:13 2021 +0100
> >
> >     ios: avoid spurious EOF when writing weird integers past the end of an 
> > IOS
> >
> >     2021-03-27  Jose E. Marchesi  <jemarch@gnu.org>
> >
> >             * testsuite/poke.map/ass-map-20.pk: New test.
> >             * testsuite/poke.map/maps-strings-4.pk: Renamed from
> >             maps-strings-diag-4.pk.
> >             * testsuite/Makefile.am (EXTRA_DIST): Update accordingly.
> >
> >
> > diff --git a/ChangeLog b/ChangeLog
> > index 5b52061e..5453b2bf 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,3 +1,10 @@
> > +2021-03-27 Jose E. Marchesi jemarch@gnu.org
> > +
> >
> > -   -   testsuite/poke.map/ass-map-20.pk: New test.
> > -   -   testsuite/poke.map/maps-strings-4.pk: Renamed from
> > -   maps-strings-diag-4.pk.
> > -   -   testsuite/Makefile.am (EXTRA_DIST): Update accordingly.
> > -
> >
> > 2021-03-27 Jose E. Marchesi jemarch@gnu.org
> > Mohammad-Reza Nabipoor m.nabipoor@yahoo.com
> > diff --git a/libpoke/ios.c b/libpoke/ios.c
> > index 75ea05de..3e04c730 100644
> > --- a/libpoke/ios.c
> > +++ b/libpoke/ios.c
> > @@ -32,13 +32,16 @@
> > #include "ios.h"
> > #include "ios-dev.h"
> > -#define IOS_GET_C_ERR_CHCK(c, io, off) \
> >
> > -   { \
> >     +#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; \
> >
> > -   int ret = (io)->dev_if->pread ((io)->dev, &ch, 1, off); \
> > -   /* If the pread reports an EOF, it means the partial byte */ \
> > -   /* we are writing is past the end of the IOS. That may or */ \
> > -   /* may not be supported in the underlying IOD, but in any */ \
> > -   /* case that will be resolved when the completed byte is */ \
> > -   /* written back. */ \
> > -   (c) = ret == IOD_EOF ? 0 : ch; \
> >     }
> >
> >
> > #define IOS_PUT_C_ERR_CHCK(c, io, len, off) \
> > diff --git a/testsuite/Makefile.am b/testsuite/Makefile.am
> > index 28968938..73b3bef4 100644
> > --- a/testsuite/Makefile.am
> > +++ b/testsuite/Makefile.am
> > @@ -124,6 +124,7 @@ EXTRA_DIST = \
> > poke.map/ass-map-17.pk \
> > poke.map/ass-map-18.pk \
> > poke.map/ass-map-19.pk \
> >
> > -   poke.map/ass-map-20.pk \
> >     poke.map/ass-map-struct-int-1.pk \
> >     poke.map/func-map-1.pk \
> >     poke.map/func-map-2.pk \
> >     @@ -276,9 +277,9 @@ EXTRA_DIST = \
> >     poke.map/maps-strings-1.pk \
> >     poke.map/maps-strings-2.pk \
> >     poke.map/maps-strings-3.pk \
> >
> > -   poke.map/maps-strings-4.pk \
> >     poke.map/maps-strings-diag-1.pk \
> >     poke.map/maps-strings-diag-2.pk \
> >
> >
> > -   poke.map/maps-strings-diag-4.pk \
> >     poke.map/maps-structs-1.pk \
> >     poke.map/maps-structs-2.pk \
> >     poke.map/maps-structs-3.pk \
> >     diff --git a/testsuite/poke.map/ass-map-20.pk 
> > b/testsuite/poke.map/ass-map-20.pk
> >     new file mode 100644
> >     index 00000000..5fb97e92
> >     --- /dev/null
> >     +++ b/testsuite/poke.map/ass-map-20.pk
> >     @@ -0,0 +1,7 @@
> >     +/* { dg-do run } */
> >
> >
> > -
> >
> > +/* { dg-command {.mem foo} } /
> > +/ { dg-command {var is = iosize} } /
> > +/ { dg-command {uint<3> @ is = 2} } /
> > +/ { dg-command {uint<3> @ is} } /
> > +/ { dg-output ".uint<3>. 2" } /
> > diff --git a/testsuite/poke.map/maps-strings-4.pk 
> > b/testsuite/poke.map/maps-strings-4.pk
> > new file mode 100644
> > index 00000000..3ced1d4c
> > --- /dev/null
> > +++ b/testsuite/poke.map/maps-strings-4.pk
> > @@ -0,0 +1,11 @@
> > +/ { dg-do run } /
> > +/ { dg-data {c*} {0x66 0x6f 0x6f 0x00 0x50 0x60 0x70 0x80 0x90 0xa0 0xb0 
> > 0xc0} } /
> > +
> > +/ Writing a string partially past the end of the IO space is allowed.
> >
> > -   The IO space grows. */
> > -
> >
> > +/* { dg-command { string @ 8#B + 1#b = "quux" } } /
> > +/ { dg-command { iosize/#B } } /
> > +/ { dg-output "13UL" } /
> > +/ { dg-command { string @ 8#B + 1#b } } /
> > +/ { dg-output "\n\"quux\"" } /
> > diff --git a/testsuite/poke.map/maps-strings-diag-4.pk 
> > b/testsuite/poke.map/maps-strings-diag-4.pk
> > deleted file mode 100644
> > index 5d7943d6..00000000
> > --- a/testsuite/poke.map/maps-strings-diag-4.pk
> > +++ /dev/null
> > @@ -1,5 +0,0 @@
> > -/ { dg-do run } /
> > -/ { dg-data {c*} {0x66 0x6f 0x6f 0x00 0x50 0x60 0x70 0x80 0x90 0xa0 0xb0 
> > 0xc0} } */
> > --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> >
> > -/* { dg-command { try string @ (8*8+1)#b = "quux"; catch if E_eof { printf 
> > "caught\n"; } } } /
> > -/ { dg-output "caught" } */