Re: [Minios-devel] [PATCH 2/2] lib/vfscore: Add both the block and echo support

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Hi Vlad-Andrei,

for the EOF, I would propose to import arch/generic/bits/termios.h from
musl as nolibc/include/termios.h.

Other then that, and 2 small notes inline the code looks good to me.

Thanks.
BR, Yuri.

"Vlad-Andrei BĂDOIU (78692)" <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
writes:

> Hey Yuri,
>
> Thank you for the review. You're right, this implementation reads
> only 1 character from stdin and it should actually read up to
> count or until \n is read. (We don't seem to have EOF defined)
>
> I've modified the code such that the busy waiting is done until we've
> read count bytes or the character \n has been read. Furthermore, for kvm
> the character \r is returned so I replaced it with \n. I have tested 
> this code on
> the micropython shell and it works. Let me know if  this looks all right to
> you before I submit the v2 patch.
>
> @@ -37,29 +37,34 @@
>   #include <uk/plat/console.h>
>   #include <uk/essentials.h>
>
> +/* One function for stderr and stdout */
> +static ssize_t stdout_write(struct vfscore_file *vfscore_file __unused,
> +                           const void *buf, size_t count)
> +{
> +       return ukplat_coutk(buf, count);
> +}
> +
>   static ssize_t stdin_read(struct vfscore_file *vfscore_file __unused,
>                            void *buf, size_t count)
>   {
> -       int read_count;
> +       int bytes_read, byes_total = 0;
It looks like a typo in byes_total

>
> -       read_count = ukplat_cink(buf, count);
> +       do {
> +               bytes_read = ukplat_cink(buf + byes_total, count - 
> byes_total);
> +               if (bytes_read <= 0)
> +                       continue;
> +               if (((char *)buf)[byes_total] == '\r')
> +                       ((char *)buf)[byes_total] = '\n';
You are here checking the previous iteration. Perhaps you meant
bytes_total+bytes_read.
>
> -#ifdef CONFIG_UK_STDIN_BLOCKING
> -       while (read_count <= 0)
> -               read_count = ukplat_cink(buf, count);
> -#endif
> +               stdout_write(vfscore_file, ((char *)buf + byes_total),
> +                                               bytes_read);
> +               byes_total += bytes_read;
>
> -#ifdef CONFIG_UK_STDIN_ECHO
> -       stdout_write(vfscore_file, buf, read_count, 0);
> -#endif
> -       return read_count;
> -}
> +       } while (byes_total == 0 || bytes_read <= 0 || (byes_total < 
> count &&
> +                       ((char *)buf)[byes_total - bytes_read] != '\n'));
>
> -/* One function for stderr and stdout */
> -static ssize_t stdout_write(struct vfscore_file *vfscore_file __unused,
> -                           const void *buf, size_t count)
> -{
>
> -       return ukplat_coutk(buf, count);
> +
> +       return byes_total;
>   }
>
>   static struct vfscore_fops stdin_fops = {
>
>
> On 12/3/18 5:24 PM, Yuri Volchkov wrote:
>> Neither CONFIG_UK_STDIN_BLOCKING, nor CONFIG_UK_STDIN_ECHO is not
>> present in any configurations. And I don't think they should. Isn't the
>> code under these ifdefs implements a bit more correct behavior of the
>> read function? Even though it would not build if I manually enable these
>> config.
>>
>> I said "more correct" because this implementation reads only 1
>> character from stdin. I think it is more reasonable to mimic the
>> whatever normal read does (read until EOF or \n met).
>>
>> That is a step in the right direction I think, but a little bit more
>> needed to be done.
>>
>> "Vlad-Andrei BĂDOIU (78692)" <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
>> writes:
>>
>>> The block behaviour is needed by the python shell. Currently
>>> uk_cink return 0 if no character is being inputted and python
>>> uses fgets on stdin which causes the console to close
>>> immediately. The echo behaviour is needed for feedback on the
>>> typed input. Both features are guarded by defines (CONFIG_UK_
>>> STDIN_BLOCKING and CONFIG_UK_STDIN_ECHO).
>>>
>>> Signed-off-by: Vlad-Andrei Badoiu <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
>>> ---
>>>   lib/vfscore/stdio.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/lib/vfscore/stdio.c b/lib/vfscore/stdio.c
>>> index 6acbb32..ec61e3a 100644
>>> --- a/lib/vfscore/stdio.c
>>> +++ b/lib/vfscore/stdio.c
>>> @@ -44,6 +44,14 @@ static ssize_t stdin_read(struct vfscore_file 
>>> *vfscore_file __unused,
>>>   
>>>     read_count = ukplat_cink(buf, count);
>>>   
>>> +#ifdef CONFIG_UK_STDIN_BLOCKING
>>> +   while (read_count <= 0)
>>> +           read_count = ukplat_cink(buf, count);
>>> +#endif
>>> +
>>> +#ifdef CONFIG_UK_STDIN_ECHO
>>> +   stdout_write(vfscore_file, buf, read_count, 0);
>>> +#endif
>>>     return read_count;
>>>   }
>>>   
>>> -- 
>>> 2.19.1
>>>

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel