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

[Alexey Kardashevskiy]

On 15/09/2023 18:34, Thomas Huth wrote:
> 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?


I just pushed it out, thanks everyone! and sorry for the delay, had to 
fix my smtp relay which I have not used for a while :)

> 
>   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
> 

-- 
Alexey