[v6,1/2] Fix output word

Message ID 20180406084704.29048-2-lvivier@redhat.com
State Superseded
Headers show
Series
  • Fix output word
Related show

Commit Message

Laurent Vivier April 6, 2018, 8:47 a.m.
We can select the console output, but it does not really work

Implement term-io-emit, as we have term-io-key to really
send characters to the output selected by stdout.

Resolve xt-handle and ihandle in the output command.

Use them in the new term-io-emit function.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 slof/fs/term-io.fs | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

Comments

Thomas Huth April 6, 2018, 9:03 a.m. | #1
On 06.04.2018 10:47, Laurent Vivier wrote:
> We can select the console output, but it does not really work
> 
> Implement term-io-emit, as we have term-io-key to really
> send characters to the output selected by stdout.
> 
> Resolve xt-handle and ihandle in the output command.
> 
> Use them in the new term-io-emit function.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  slof/fs/term-io.fs | 36 +++++++++++++++++++++++++++++++-----
>  1 file changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/slof/fs/term-io.fs b/slof/fs/term-io.fs
> index 52ce12a..d247b09 100644
> --- a/slof/fs/term-io.fs
> +++ b/slof/fs/term-io.fs
> @@ -10,6 +10,17 @@
>  \ *     IBM Corporation - initial implementation
>  \ ****************************************************************************/
>  
> +0 VALUE write-xt
> +
> +VARIABLE stdout
> +
> +: set-stdout ( ihandle -- )
> +   \ Close old stdout:
> +   stdout @ ?dup IF close-dev THEN
> +   \ Now set the new stdout:
> +   dup stdout !
> +   encode-int s" stdout" set-chosen
> +;
>  
>  : input  ( dev-str dev-len -- )
>     open-dev ?dup IF
> @@ -24,12 +35,15 @@
>  
>  : output  ( dev-str dev-len -- )
>     open-dev ?dup IF
> -      \ Close old stdout:
> -      s" stdout" get-chosen IF
> -         decode-int nip nip ?dup IF close-dev THEN
> +      \ find new ihandle and xt handle
> +      dup s" write" rot ihandle>phandle find-method
> +      0= IF
> +         drop
> +         cr ." Cannot find the write method for the given output console " cr
> +         EXIT
>        THEN
> -      \ Now set the new stdout:
> -      encode-int s" stdout" set-chosen
> +      to write-xt
> +      set-stdout
>     THEN
>  ;
>  
> @@ -40,6 +54,18 @@
>  
>  1 BUFFER: (term-io-char-buf)
>  
> +: term-io-emit ( char -- )
> +    write-xt IF
> +       (term-io-char-buf) c!
> +       (term-io-char-buf) 1 write-xt stdout @ call-package
> +       drop
> +    ELSE
> +       [ ' emit behavior compile, ]
> +    THEN
> +;
> +
> +' term-io-emit to emit
> +
>  : term-io-key  ( -- char )
>     s" stdin" get-chosen IF
>        decode-int nip nip dup 0= IF 0 EXIT THEN
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>
Segher Boessenkool April 8, 2018, 11:05 a.m. | #2
Hi!

On Fri, Apr 06, 2018 at 10:47:03AM +0200, Laurent Vivier wrote:
>  : output  ( dev-str dev-len -- )
>     open-dev ?dup IF

It's a lot simple if you do an early exit if there is no such device.
You're supposed to output an error message in that case, fwiw.

> -      \ Close old stdout:
> -      s" stdout" get-chosen IF
> -         decode-int nip nip ?dup IF close-dev THEN
> +      \ find new ihandle and xt handle
> +      dup s" write" rot ihandle>phandle find-method
> +      0= IF
> +         drop
> +         cr ." Cannot find the write method for the given output console " cr
> +         EXIT

This leaks the instance.  The description in the standard say to search
for the method before doing the open-dev, that solves this problem too.

> +: term-io-emit ( char -- )
> +    write-xt IF
> +       (term-io-char-buf) c!
> +       (term-io-char-buf) 1 write-xt stdout @ call-package
> +       drop
> +    ELSE
> +       [ ' emit behavior compile, ]
> +    THEN
> +;

What do you expect to be in the emit hook at this point?  Could you
just use _that_?

Looks nice and readable otherwise, great :-)


Segher
Thomas Huth April 9, 2018, 6:15 a.m. | #3
On 08.04.2018 13:05, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Apr 06, 2018 at 10:47:03AM +0200, Laurent Vivier wrote:
>>  : output  ( dev-str dev-len -- )
>>     open-dev ?dup IF
> 
> It's a lot simple if you do an early exit if there is no such device.
> You're supposed to output an error message in that case, fwiw.
> 
>> -      \ Close old stdout:
>> -      s" stdout" get-chosen IF
>> -         decode-int nip nip ?dup IF close-dev THEN
>> +      \ find new ihandle and xt handle
>> +      dup s" write" rot ihandle>phandle find-method
>> +      0= IF
>> +         drop
>> +         cr ." Cannot find the write method for the given output console " cr
>> +         EXIT
> 
> This leaks the instance.  The description in the standard say to search
> for the method before doing the open-dev, that solves this problem too.
> 
>> +: term-io-emit ( char -- )
>> +    write-xt IF
>> +       (term-io-char-buf) c!
>> +       (term-io-char-buf) 1 write-xt stdout @ call-package
>> +       drop
>> +    ELSE
>> +       [ ' emit behavior compile, ]
>> +    THEN
>> +;
> 
> What do you expect to be in the emit hook at this point?  Could you
> just use _that_?

The code in slof/fs/term-io.fs is "common" code, i.e. shared between all
"boards", so there is no 100% generic way to say which token should be
expected in the emit hook at this point in time. That's why we have the
detour with [ ' ... behavior compile, ] here. Unless we want to
hard-code a default name for all boards (ok, we currently only have two
boards, but still...), I think that line is a nice and short solution to
the problem.

 Thomas
Segher Boessenkool April 9, 2018, 11:21 a.m. | #4
On Mon, Apr 09, 2018 at 08:15:37AM +0200, Thomas Huth wrote:
> On 08.04.2018 13:05, Segher Boessenkool wrote:
> >> +    ELSE
> >> +       [ ' emit behavior compile, ]
> >> +    THEN
> >> +;
> > 
> > What do you expect to be in the emit hook at this point?  Could you
> > just use _that_?
> 
> The code in slof/fs/term-io.fs is "common" code, i.e. shared between all
> "boards", so there is no 100% generic way to say which token should be
> expected in the emit hook at this point in time. That's why we have the
> detour with [ ' ... behavior compile, ] here. Unless we want to
> hard-code a default name for all boards (ok, we currently only have two
> boards, but still...), I think that line is a nice and short solution to
> the problem.

The usual solution is to just have two hooks, and have one call the
other.  So

DEFER debug-emit

...
  ELSE debug-emit THEN ;

(or call it early-emit, or whatever makes sense).


Segher
Thomas Huth April 9, 2018, 11:54 a.m. | #5
On 09.04.2018 13:21, Segher Boessenkool wrote:
> On Mon, Apr 09, 2018 at 08:15:37AM +0200, Thomas Huth wrote:
>> On 08.04.2018 13:05, Segher Boessenkool wrote:
>>>> +    ELSE
>>>> +       [ ' emit behavior compile, ]
>>>> +    THEN
>>>> +;
>>>
>>> What do you expect to be in the emit hook at this point?  Could you
>>> just use _that_?
>>
>> The code in slof/fs/term-io.fs is "common" code, i.e. shared between all
>> "boards", so there is no 100% generic way to say which token should be
>> expected in the emit hook at this point in time. That's why we have the
>> detour with [ ' ... behavior compile, ] here. Unless we want to
>> hard-code a default name for all boards (ok, we currently only have two
>> boards, but still...), I think that line is a nice and short solution to
>> the problem.
> 
> The usual solution is to just have two hooks, and have one call the
> other.  So
> 
> DEFER debug-emit
> 
> ...
>   ELSE debug-emit THEN ;
> 
> (or call it early-emit, or whatever makes sense).

Looks like we've already got serial-key, serial-key? and serial-emit on
both board-js2x and board-qemu, so I assume Laurent could simply use
these here...

 Thomas
Alexey Kardashevskiy May 14, 2018, 9:55 a.m. | #6
On 8/4/18 9:05 pm, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Apr 06, 2018 at 10:47:03AM +0200, Laurent Vivier wrote:
>>  : output  ( dev-str dev-len -- )
>>     open-dev ?dup IF
> 
> It's a lot simple if you do an early exit if there is no such device.
> You're supposed to output an error message in that case, fwiw.
> 
>> -      \ Close old stdout:
>> -      s" stdout" get-chosen IF
>> -         decode-int nip nip ?dup IF close-dev THEN
>> +      \ find new ihandle and xt handle
>> +      dup s" write" rot ihandle>phandle find-method
>> +      0= IF
>> +         drop
>> +         cr ." Cannot find the write method for the given output console " cr
>> +         EXIT
> 
> This leaks the instance.


Laurent, you are reposting this patch with a fix, right? thanks.


> The description in the standard say to search
> for the method before doing the open-dev, that solves this problem too.
> 
>> +: term-io-emit ( char -- )
>> +    write-xt IF
>> +       (term-io-char-buf) c!
>> +       (term-io-char-buf) 1 write-xt stdout @ call-package
>> +       drop
>> +    ELSE
>> +       [ ' emit behavior compile, ]
>> +    THEN
>> +;
> 
> What do you expect to be in the emit hook at this point?  Could you
> just use _that_?
> 
> Looks nice and readable otherwise, great :-)
> 
> 
> Segher
> _______________________________________________
> SLOF mailing list
> SLOF@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/slof
>
Laurent Vivier May 22, 2018, 8:47 a.m. | #7
On 14/05/2018 11:55, Alexey Kardashevskiy wrote:
> On 8/4/18 9:05 pm, Segher Boessenkool wrote:
>> Hi!
>>
>> On Fri, Apr 06, 2018 at 10:47:03AM +0200, Laurent Vivier wrote:
>>>  : output  ( dev-str dev-len -- )
>>>     open-dev ?dup IF
>>
>> It's a lot simple if you do an early exit if there is no such device.
>> You're supposed to output an error message in that case, fwiw.
>>
>>> -      \ Close old stdout:
>>> -      s" stdout" get-chosen IF
>>> -         decode-int nip nip ?dup IF close-dev THEN
>>> +      \ find new ihandle and xt handle
>>> +      dup s" write" rot ihandle>phandle find-method
>>> +      0= IF
>>> +         drop
>>> +         cr ." Cannot find the write method for the given output console " cr
>>> +         EXIT
>>
>> This leaks the instance.
> 
> 
> Laurent, you are reposting this patch with a fix, right? thanks.

Alexey, I've sent a v7 last week. It has been reviewed by Thomas.
Regarding the comments from Segher, I will not change the code because
review is clearer like that (not indentation changes), but if you commit
this series could you update the comment as he asks ("xt" instead of
"xt-handle")?

Thanks,
Laurent

Patch

diff --git a/slof/fs/term-io.fs b/slof/fs/term-io.fs
index 52ce12a..d247b09 100644
--- a/slof/fs/term-io.fs
+++ b/slof/fs/term-io.fs
@@ -10,6 +10,17 @@ 
 \ *     IBM Corporation - initial implementation
 \ ****************************************************************************/
 
+0 VALUE write-xt
+
+VARIABLE stdout
+
+: set-stdout ( ihandle -- )
+   \ Close old stdout:
+   stdout @ ?dup IF close-dev THEN
+   \ Now set the new stdout:
+   dup stdout !
+   encode-int s" stdout" set-chosen
+;
 
 : input  ( dev-str dev-len -- )
    open-dev ?dup IF
@@ -24,12 +35,15 @@ 
 
 : output  ( dev-str dev-len -- )
    open-dev ?dup IF
-      \ Close old stdout:
-      s" stdout" get-chosen IF
-         decode-int nip nip ?dup IF close-dev THEN
+      \ find new ihandle and xt handle
+      dup s" write" rot ihandle>phandle find-method
+      0= IF
+         drop
+         cr ." Cannot find the write method for the given output console " cr
+         EXIT
       THEN
-      \ Now set the new stdout:
-      encode-int s" stdout" set-chosen
+      to write-xt
+      set-stdout
    THEN
 ;
 
@@ -40,6 +54,18 @@ 
 
 1 BUFFER: (term-io-char-buf)
 
+: term-io-emit ( char -- )
+    write-xt IF
+       (term-io-char-buf) c!
+       (term-io-char-buf) 1 write-xt stdout @ call-package
+       drop
+    ELSE
+       [ ' emit behavior compile, ]
+    THEN
+;
+
+' term-io-emit to emit
+
 : term-io-key  ( -- char )
    s" stdin" get-chosen IF
       decode-int nip nip dup 0= IF 0 EXIT THEN