diff mbox series

[2/3] virtio-serial: Make read and write methods report failure

Message ID 20230828013736.18414-2-jniethe5@gmail.com
State Superseded
Headers show
Series [1/3] virtio-serial: Fix invalid stack access with closed virtio device | expand

Commit Message

Jordan Niethe Aug. 28, 2023, 1:37 a.m. UTC
From: Kautuk Consul <kconsul@linux.vnet.ibm.com>

The read and write methods return as though they were 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.

Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
 board-qemu/slof/virtio-serial.fs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Thomas Huth Aug. 28, 2023, 7:01 a.m. UTC | #1
On 28/08/2023 03.37, Jordan Niethe wrote:
> From: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> 
> The read and write methods return as though they were successfully even

I'd scratch "as though they were" from the sentence - that part makes it 
just harder to read IMHO.

> 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.
> 
> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
>   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 a67a310..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

Ok, makes sense!

>       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 drop 1 EXIT THEN
> +    virtiodev 0= IF drop 0 EXIT THEN

Ah, ok, here's the 0 ... I'd suggest to use that in the first patch already.

  Thomas
Jordan Niethe Aug. 28, 2023, 8:06 a.m. UTC | #2
On Mon, Aug 28, 2023 at 5:01 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 28/08/2023 03.37, Jordan Niethe wrote:
> > From: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> >
> > The read and write methods return as though they were successfully even
>
> I'd scratch "as though they were" from the sentence - that part makes it
> just harder to read IMHO.

Sure.

>
> > 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.
> >
> > Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> >   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 a67a310..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
>
> Ok, makes sense!
>
> >       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 drop 1 EXIT THEN
> > +    virtiodev 0= IF drop 0 EXIT THEN
>
> Ah, ok, here's the 0 ... I'd suggest to use that in the first patch already.

Just wanted to seperate fixing the incorrectly implemented desired
behaviour of commit 8174acd ("virtio-serial: Close device completely")
in which the virtiodev == 0 case returns successfully and changing
that to return unsuccessfully, to check if there was some other
situation Alexey had had in mind for that behaviour.

First patch can be dropped.

>
>   Thomas
>
> _______________________________________________
> SLOF mailing list
> SLOF@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/slof
diff mbox series

Patch

diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs
index a67a310..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 drop 1 EXIT THEN
+    virtiodev 0= IF drop 0 EXIT THEN
     virtiodev virtio-serial-haschar 0= IF 0 swap c! -2 EXIT THEN
     virtiodev virtio-serial-getchar swap c! 1
 ;