Re: [PATCH] ios: Introduce IO stream device for stdin, stdout, stderr.

[Egeyar Bagcioglu]

Hello!

The updated patch is attached. Here is the corrected ChangeLog:

2020-10-11  Egeyar Bagcioglu  <egeyar@gmail.com>

        * libpoke/Makefile.am (libpoke_la_SOURCES): Add ios-buffer.h,
        ios-buffer.c and ios-dev-stream.c.
        * libpoke/ios-buffer.c: New file.
        * libpoke/ios-buffer.h: New file.
        * libpoke/ios-dev-stream.c: New file.
        * libpoke/ios.c: Extern ios_dev_stream.
        (ios_dev_ifs): Add ios_dev_stream.


On 7/12/20 5:58 PM, Jose E. Marchesi wrote:
> Hi Egeyar.
>      
>      Hereby I submit the support for stdin, stdout, stderr with the following
>      changes that you asked for:
>      * Moving ios-dev-buffer.h to ios-buffer.h
>      * Change the handlers of the stdio.
>      * Converting stdin to read-only, converting stdout and stderr to 
> write-only.
>
>      We also talked about adding suport for device files. I need to keep that
>      out of this commit, so that I can forget about the stdio while implementing
>      that. Otherwise, my brain starts paging out things.
>
> Thanks :)
>
>      Regards
>      Egeyar
>      
>      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      
>      The handlers for stdin, stdout and stderr are these words surrounded by '<'
>      and '>' characters. open("<stdin>") for example opens stdin.
>
> Perfect, it is very unlikely real files will have such names, and we can
> use the same convention for additional "pre-defined" IO spaces.
>
>      Upon opening, a read stream device initializes a buffer to store the read
>      data. It was discussed during the first poke-conf that such a buffer should
>      not be kept as one piece, as this might easily make copying the whole 
> buffer
>      very expensive during expanding and flushing (i.e. discarding the early 
> parts
>      of the buffer). Therefore, the buffer is implemented in chunks. These 
> chunks
>      are kept in an hashtable -if you may- where the hash function is simply
>      chunk_no % IOB_BUCKET_COUNT.
>      
>      Write streams do not use a buffer. Instead they maintain an offset. Trying
>      to rewrite to an already written offset results in an error. Writing to a
>      greater offset while leaving a gap, results in the gap being filled with 0s
>      (0 bytes, not '0').
>      
>      Currently, ios_dev_stream_flush still requires language level
>      support.
>
> I am working on `forget' right now :)
>      
>      2020-07-11  Egeyar Bagcioglu<egeyar@gmail.com>
>      
>              * libpoke/Makefile.am (libpoke_la_SOURCES): Add ios-dev-buffer.h
>         and ios-dev-stream.c.
>
> ios-buffer.h

right!

>              * libpoke/ios-buffer.h: New file.
>              * libpoke/ios-dev-stream.c: New file.
>              * libpoke/ios.c: Extern ios_dev_stream.
>              (ios_dev_ifs): Add ios_dev_stream.
>      ---
>       libpoke/Makefile.am      |   3 +-
>       libpoke/ios-buffer.h     | 249 +++++++++++++++++++++++++++++++++++++++
>       libpoke/ios-dev-stream.c | 232 ++++++++++++++++++++++++++++++++++++
>       libpoke/ios.c            |   2 +
>       4 files changed, 485 insertions(+), 1 deletion(-)
>       create mode 100644 libpoke/ios-buffer.h
>       create mode 100644 libpoke/ios-dev-stream.c
>      
>      diff --git a/libpoke/Makefile.am b/libpoke/Makefile.am
>      index bab9793f..3a910b82 100644
>      --- a/libpoke/Makefile.am
>      +++ b/libpoke/Makefile.am
>      @@ -55,7 +55,8 @@ libpoke_la_SOURCES = libpoke.h libpoke.c \
>                            pvm-program.h pvm-program.c \
>                            pvm.jitter \
>                            ios.c ios.h ios-dev.h \
>      -                     ios-dev-file.c ios-dev-mem.c
>      +                     ios-dev-file.c ios-dev-mem.c \
>      +                     ios-dev-buffer.h ios-dev-stream.c
>
> That should be ios-buffer.h?

yep!

>       
>       libpoke_la_SOURCES += ../common/pk-utils.c ../common/pk-utils.h
>       
>      diff --git a/libpoke/ios-buffer.h b/libpoke/ios-buffer.h
>      new file mode 100644
>      index 00000000..9a52a56a
>      --- /dev/null
>      +++ b/libpoke/ios-buffer.h
>      @@ -0,0 +1,249 @@
>      +/* ios-buffer.h - The buffer for IO devices.  */
>
> Isn't this file intended to be called ios-dev-buffer.h instead?
> Ditto for the associated #includes below.

I intended so earlier. You asked for it to be changed to ios-buffer.h 
instead, which I agree with. ios-dev-buffer sounds as if this was a IOD 
for buffers.

>      +
>      +/* Copyright (C) 2020 Egeyar Bagcioglu */
>      +
>      +/* This program is free software: you can redistribute it and/or modify
>      + * it under the terms of the GNU General Public License as published by
>      + * the Free Software Foundation, either version 3 of the License, or
>      + * (at your option) any later version.
>      + *
>      + * This program is distributed in the hope that it will be useful,
>      + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>      + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>      + * GNU General Public License for more details.
>      + *
>      + * You should have received a copy of the GNU General Public License
>      + * along with this program.  If not, see<http://www.gnu.org/licenses/>.
>      + */
>      +
>      +#include <config.h>
>      +#include <stdlib.h>
>      +#include <unistd.h>
>      +#include <malloc.h>
>      +#include <assert.h>
>      +
>      +#define IOB_CHUNK_SIZE            2048
>      +#define IOB_BUCKET_COUNT  8
>      +
>      +#define IOB_CHUNK_OFFSET(offset)  \
>      +  ((offset) % IOB_CHUNK_SIZE)
>      +
>      +#define IOB_CHUNK_NO(offset)              \
>      +  ((offset) / IOB_CHUNK_SIZE)
>      +
>      +#define IOB_BUCKET_NO(chunk_no)           \
>      +  ((chunk_no) % IOB_BUCKET_COUNT)
>      +
>      +typedef struct ios_buffer_chunk
>      +{
>      +  uint8_t bytes[IOB_CHUNK_SIZE];
>      +  int chunk_no;
>      +  struct ios_buffer_chunk *next;
>      +} ios_buffer_chunk;
>      +
>      +/* begin_offset is the first offset that's not yet flushed, initilized as 
> 0.
>      +   end_offset of an instream is the next byte to read to.  end_offset of 
> an
>      +   outstream is the successor of the greatest offset that is written to.  
> */
>      +
>      +typedef struct ios_buffer
>      +{
>      +  ios_buffer_chunk* chunks[IOB_BUCKET_COUNT];
>      +  ios_dev_off begin_offset;
>      +  ios_dev_off end_offset;
>      +  int next_chunk_no;
>      +} ios_buffer;
>
> In poke we usually use typedef in order to abstract pointer types, i.e.:
>
> typedef struct ios_buffer
> {
>    ios_buffer_chunk* chunks[IOB_BUCKET_COUNT];
>    ios_dev_off begin_offset;
>    ios_dev_off end_offset;
>    int next_chunk_no;
> };
>
> typedef struct ios_buffer *ios_buffer;

I would surely confuse what is a pointer or not with this schema. So I 
go with the second option below.

> Otherwise I find it is much clearer to use the struct tag, like in:
> struct ios_buffer *buffer.

Done.

> Also, I wonder if it would be a good idea to move the type definition
> and the functions ios_buffer_* into a ios-buffer.c.  This way,
> ios-buffer.h would export an opaque type, and we wouldn't be replicating
> code if/when ios-buffer gets used somewhere else...

Done!

>      +static ios_buffer *
>      +ios_buffer_init ()
>      +{
>      +  ios_buffer *bio = calloc (1, sizeof (ios_buffer));
>      +  return bio;
>      +}
>      +
>      +static int
>      +ios_buffer_free (ios_buffer *buffer)
>      +{
>      +  ios_buffer_chunk *chunk, *chunk_next;
>      +  for (int i = 0; i < IOB_BUCKET_COUNT; i++)
>      +    {
>      +      chunk = buffer->chunks[i];
>      +      while (chunk)
>      +  {
>      +    chunk_next = chunk->next;
>      +    free (chunk);
>      +    chunk = chunk_next;
>      +  }
>      +    }
>      +
>      +  free (buffer);
>      +  return 1;
>      +}
>      +
>      +static ios_buffer_chunk*
>      +ios_buffer_get_chunk (ios_buffer *buffer, int chunk_no)
>      +{
>      +  int bucket_no = IOB_BUCKET_NO (chunk_no);
>      +  ios_buffer_chunk *chunk = buffer->chunks[bucket_no];
>      +
>      +  for ( ; chunk; chunk = chunk->next)
>      +    if (chunk->chunk_no == chunk_no)
>      +      return chunk;
>      +
>      +  return NULL;
>      +}
>      +
>      +static int
>      +ios_buffer_allocate_new_chunk (ios_buffer *buffer, int final_chunk_no,
>      +                         ios_buffer_chunk **final_chunk)
>      +{
>      +  ios_buffer_chunk *chunk;
>      +  int bucket_no;
>      +
>      +  assert (buffer->next_chunk_no <= final_chunk_no);
>      +
>      +  do
>      +    {
>      +      chunk = calloc(1, sizeof (ios_buffer_chunk));
>
> Space between calloc and (.

Done.

Regards,
Ege

ios-dev-stream.patch
Description: Text Data