[SLOF] [PATCH v2 1/2] virtio-serial: Make read and write methods report failure

[Thomas Huth]

On 08/09/2023 08.43, Jordan Niethe wrote:
> On Thu, Sep 7, 2023 at 1:37 PM Alexey Kardashevskiy <aik at ozlabs.ru> wrote:
>> On 04/09/2023 15:19, Jordan Niethe wrote:
>>> On Mon, Sep 4, 2023 at 3:08 PM Alexey Kardashevskiy <aik at ozlabs.ru> wrote:
...
>>>>> diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs
>>>>> index 82868e2..41e2e04 100644
>>>>> --- a/board-qemu/slof/virtio-serial.fs
>>>>> +++ b/board-qemu/slof/virtio-serial.fs
>>>>> @@ -67,7 +67,7 @@ virtiodev virtio-serial-init drop
>>>>>     ;
>>>>>
>>>>>     : write ( addr len -- actual )
>>>>> -    virtiodev 0= IF nip EXIT THEN
>>>>> +    virtiodev 0= IF 2drop 0 EXIT THEN
>>>>>         tuck
>>>>>         0 ?DO
>>>>>             dup c@ virtiodev SWAP virtio-serial-putchar
>>>>> @@ -78,7 +78,7 @@ virtiodev virtio-serial-init drop
>>>>>
>>>>>     : read ( addr len -- actual )
>>>>>         0= IF drop 0 EXIT THEN
>>>>> -    virtiodev 0= IF nip EXIT THEN
>>>>> +    virtiodev 0= IF drop 0 EXIT THEN
>>>>
>>>>
>>>> This is going to leave @addr on stack, no? The write gets it right though.
>>>
>>> The read method begins with 0= which consumes top of stack.
>>> The write method does not have that which is why they are different.
>>
>> ah, you're right. I dislike forth :)
>>
>> btw since now these do report failures (or, to be precise, 0 chars
>> written/read which is not exactly an error),
> 
> I think it can be described as reporting failure based on the ieee1275
> descriptions of
> read and write:
> 
> - "If actual is zero or negative, the read operation did not succeed"
> - "If actual is less than len, the write did not succeed"

I agree. It's not like in libc where they have a slightly different meaning 
of the return values. It's fine for Open Firmware as it is done in this 
patch, as far as I can tell.

>> what effect does it have?
>> For example, something is writing a buffer in a loop to a device and
>> incrementing some counter by the value returned by "write" and now it
>> returns 0 instead of passed "len", or there is nothing like this?
> 
> I was worried about that too. The v1 of the series fixed the stack
> misaccess but still reported success.
> 
> AFAICS, that only callers of the read and write methods are
> term-io-key and term-io-emit.
> term-io-emit ignores the return value of the write method.
> term-io-key will call read until it returns >= 0.
> 
> So if "read" is called with the virtiodev == 0 it will loop forever.
> 
> prom_init closes stdin prior to quiescing so it won't happen there,
> but it is conceivable of a client that calls quiesce and then tries to
> read from stdin and then gets trapped.
> 
> We could add some circuit breaker to term-io-key?
> Or just keep the read and write methods reporting success?

I think we should be fine. "term-io-key" is supposed to block until there is 
a real key pressed. If a caller wants to avoid blocking, they have to use 
the "term-io-key?" (or rather "key?") Forth word first to check whether a 
key is available or not, and that should give the proper return code (0 for 
no key available).

Thus IMHO these two patches should be fine for inclusion now. Or, Alexey, 
what do you think?

  Thomas


PS: A colleague of mine just ran into the same issue and confirmed that the 
patches are fixes the problem in his case, too:
https://issues.redhat.com/browse/RHEL-3709