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

[Jordan Niethe]

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:
> >>
> >>
> >> On 29/08/2023 10:12, Jordan Niethe wrote:
> >>> From: Kautuk Consul <kconsul at linux.vnet.ibm.com>
> >>>
> >>> The read and write methods return successfully even if the virtio device
> >>> is closed (virtiodev is 0) and it is not able to send or receive any
> >>> characters.
> >>>
> >>> Make the read and write methods return 0 to indicate they did not
> >>> succeed in this case.
> >>>
> >>> This also fixes an invalid stack access in the read method.
> >>>
> >>> Fixes: 8174acd ("virtio-serial: Close device completely")
> >>> Signed-off-by: Kautuk Consul <kconsul at linux.vnet.ibm.com>
> >>> Signed-off-by: Jordan Niethe <jniethe5 at gmail.com>
> >>> ---
> >>> v2:
> >>>      - Rework commit message slightly
> >>> ---
> >>>    board-qemu/slof/virtio-serial.fs | 4 ++--
> >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> 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"

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


>
> >
> >>
> >>
> >>>        virtiodev virtio-serial-haschar 0= IF 0 swap c! -2 EXIT THEN
> >>>        virtiodev virtio-serial-getchar swap c! 1
> >>>    ;
> >>
> >> --
> >> Alexey
> >>
> >>
>
> --
> Alexey
>
>