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

[Jordan Niethe]

On Mon, Sep 18, 2023 at 8:18 PM Alexey Kardashevskiy <aik at ozlabs.ru> wrote:
>
>
> 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 :)

Thanks Alexey.

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