diff mbox series

[PULL,52/53] char: Remove unwanted crlf conversion

Message ID 20180531171606.21604-14-pbonzini@redhat.com
State New
Headers show
Series [PULL,01/53] sandbox: disable -sandbox if CONFIG_SECCOMP undefined | expand

Commit Message

Paolo Bonzini May 31, 2018, 5:16 p.m. UTC 
From: Patryk Olszewski <patryk@fala.ehost.pl>

This patch fixes a bug in serial that made it almost impossible for guest
to communicate with devices through host's serial.

OPOST flag in c_oflag enables output processing letting other flags in
c_oflag take effect. Usually in c_oflag ONLCR flag is also set, which
causes crlf to be sent in place of lf. This breaks binary transmissions.
Unsetting OPOST flag turns off any output processing which fixes the bug.

Bug reports related:
https://bugs.launchpad.net/qemu/+bug/1772086
https://bugs.launchpad.net/qemu/+bug/1407813
https://bugs.launchpad.net/qemu/+bug/1715296
also
https://lists.nongnu.org/archive/html/qemu-devel/2006-06/msg00196.html

Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>
Message-Id: <1527105041-21013-1-git-send-email-patryk@fala.ehost.pl>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 chardev/char-serial.c | 2 +-
 chardev/char-stdio.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Greg Kurz June 8, 2018, 5:39 p.m. UTC | #1 
On Thu, 31 May 2018 19:16:05 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> From: Patryk Olszewski <patryk@fala.ehost.pl>
> 
> This patch fixes a bug in serial that made it almost impossible for guest
> to communicate with devices through host's serial.
> 
> OPOST flag in c_oflag enables output processing letting other flags in
> c_oflag take effect. Usually in c_oflag ONLCR flag is also set, which
> causes crlf to be sent in place of lf. This breaks binary transmissions.
> Unsetting OPOST flag turns off any output processing which fixes the bug.
> 

But it damages error reporting...

Without this patch:

$ qemu-system-ppc64 -serial stdio -kernel foo
foo: No such file or directory
qemu-system-ppc64: error loading foo: Failed to load ELF
$

With this patch:

$ .mbuild-ppc-for-3.0/obj/ppc64-softmmu/qemu-system-ppc64 -serial stdio -kernel foo
foo: No such file or directory
                              qemu-system-ppc64: error loading foo: Failed to load ELF
                                                                                      $

It is possible to patch vreport() to append an explicit CR:

     error_vprintf(fmt, ap);
-    error_printf("\n");
+    error_printf("\n\r");
 }

but it only fixes the trailing newline of error_report(). Any other newline,
eg when using error_append_hint(), will lack the CR... Not sure how to fix
this :-\

> Bug reports related:
> https://bugs.launchpad.net/qemu/+bug/1772086
> https://bugs.launchpad.net/qemu/+bug/1407813
> https://bugs.launchpad.net/qemu/+bug/1715296
> also
> https://lists.nongnu.org/archive/html/qemu-devel/2006-06/msg00196.html
> 
> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>
> Message-Id: <1527105041-21013-1-git-send-email-patryk@fala.ehost.pl>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  chardev/char-serial.c | 2 +-
>  chardev/char-stdio.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/chardev/char-serial.c b/chardev/char-serial.c
> index feb52e559d..ae548d28da 100644
> --- a/chardev/char-serial.c
> +++ b/chardev/char-serial.c
> @@ -139,7 +139,7 @@ static void tty_serial_init(int fd, int speed,
>  
>      tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
>                       | INLCR | IGNCR | ICRNL | IXON);
> -    tty.c_oflag |= OPOST;
> +    tty.c_oflag &= ~OPOST;
>      tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN | ISIG);
>      tty.c_cflag &= ~(CSIZE | PARENB | PARODD | CRTSCTS | CSTOPB);
>      switch (data_bits) {
> diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c
> index 96375f2ab8..d83e60e787 100644
> --- a/chardev/char-stdio.c
> +++ b/chardev/char-stdio.c
> @@ -59,7 +59,7 @@ static void qemu_chr_set_echo_stdio(Chardev *chr, bool echo)
>      if (!echo) {
>          tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
>                           | INLCR | IGNCR | ICRNL | IXON);
> -        tty.c_oflag |= OPOST;
> +        tty.c_oflag &= ~OPOST;
>          tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN);
>          tty.c_cflag &= ~(CSIZE | PARENB);
>          tty.c_cflag |= CS8;
Philippe Mathieu-Daudé June 8, 2018, 5:56 p.m. UTC | #2 
On 06/08/2018 02:39 PM, Greg Kurz wrote:
> On Thu, 31 May 2018 19:16:05 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> From: Patryk Olszewski <patryk@fala.ehost.pl>
>>
>> This patch fixes a bug in serial that made it almost impossible for guest
>> to communicate with devices through host's serial.
>>
>> OPOST flag in c_oflag enables output processing letting other flags in
>> c_oflag take effect. Usually in c_oflag ONLCR flag is also set, which
>> causes crlf to be sent in place of lf. This breaks binary transmissions.
>> Unsetting OPOST flag turns off any output processing which fixes the bug.
>>
> 
> But it damages error reporting...
> 
> Without this patch:
> 
> $ qemu-system-ppc64 -serial stdio -kernel foo
> foo: No such file or directory
> qemu-system-ppc64: error loading foo: Failed to load ELF
> $
> 
> With this patch:
> 
> $ .mbuild-ppc-for-3.0/obj/ppc64-softmmu/qemu-system-ppc64 -serial stdio -kernel foo
> foo: No such file or directory
>                               qemu-system-ppc64: error loading foo: Failed to load ELF
>                                                                                       $
> 
> It is possible to patch vreport() to append an explicit CR:
> 
>      error_vprintf(fmt, ap);
> -    error_printf("\n");
> +    error_printf("\n\r");
>  }
> 
> but it only fixes the trailing newline of error_report(). Any other newline,
> eg when using error_append_hint(), will lack the CR... Not sure how to fix
> this :-\

Peter just pushed the fix (ed6b018ef7):

http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg02152.html

> 
>> Bug reports related:
>> https://bugs.launchpad.net/qemu/+bug/1772086
>> https://bugs.launchpad.net/qemu/+bug/1407813
>> https://bugs.launchpad.net/qemu/+bug/1715296
>> also
>> https://lists.nongnu.org/archive/html/qemu-devel/2006-06/msg00196.html
>>
>> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>
>> Message-Id: <1527105041-21013-1-git-send-email-patryk@fala.ehost.pl>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  chardev/char-serial.c | 2 +-
>>  chardev/char-stdio.c  | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/chardev/char-serial.c b/chardev/char-serial.c
>> index feb52e559d..ae548d28da 100644
>> --- a/chardev/char-serial.c
>> +++ b/chardev/char-serial.c
>> @@ -139,7 +139,7 @@ static void tty_serial_init(int fd, int speed,
>>  
>>      tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
>>                       | INLCR | IGNCR | ICRNL | IXON);
>> -    tty.c_oflag |= OPOST;
>> +    tty.c_oflag &= ~OPOST;
>>      tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN | ISIG);
>>      tty.c_cflag &= ~(CSIZE | PARENB | PARODD | CRTSCTS | CSTOPB);
>>      switch (data_bits) {
>> diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c
>> index 96375f2ab8..d83e60e787 100644
>> --- a/chardev/char-stdio.c
>> +++ b/chardev/char-stdio.c
>> @@ -59,7 +59,7 @@ static void qemu_chr_set_echo_stdio(Chardev *chr, bool echo)
>>      if (!echo) {
>>          tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
>>                           | INLCR | IGNCR | ICRNL | IXON);
>> -        tty.c_oflag |= OPOST;
>> +        tty.c_oflag &= ~OPOST;
>>          tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN);
>>          tty.c_cflag &= ~(CSIZE | PARENB);
>>          tty.c_cflag |= CS8;
>
Patryk Olszewski June 8, 2018, 6:08 p.m. UTC | #3 
W dniu 08.06.2018 o 19:39, Greg Kurz pisze:
> On Thu, 31 May 2018 19:16:05 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> From: Patryk Olszewski <patryk@fala.ehost.pl>
>>
>> This patch fixes a bug in serial that made it almost impossible for guest
>> to communicate with devices through host's serial.
>>
>> OPOST flag in c_oflag enables output processing letting other flags in
>> c_oflag take effect. Usually in c_oflag ONLCR flag is also set, which
>> causes crlf to be sent in place of lf. This breaks binary transmissions.
>> Unsetting OPOST flag turns off any output processing which fixes the bug.
>>
> But it damages error reporting...
>
> Without this patch:
>
> $ qemu-system-ppc64 -serial stdio -kernel foo
> foo: No such file or directory
> qemu-system-ppc64: error loading foo: Failed to load ELF
> $
>
> With this patch:
>
> $ .mbuild-ppc-for-3.0/obj/ppc64-softmmu/qemu-system-ppc64 -serial stdio -kernel foo
> foo: No such file or directory
>                               qemu-system-ppc64: error loading foo: Failed to load ELF
>                                                                                       $
>
> It is possible to patch vreport() to append an explicit CR:
>
>      error_vprintf(fmt, ap);
> -    error_printf("\n");
> +    error_printf("\n\r");
>  }
>
> but it only fixes the trailing newline of error_report(). Any other newline,
> eg when using error_append_hint(), will lack the CR... Not sure how to fix
> this :-\
>
>> Bug reports related:
>> https://bugs.launchpad.net/qemu/+bug/1772086
>> https://bugs.launchpad.net/qemu/+bug/1407813
>> https://bugs.launchpad.net/qemu/+bug/1715296
>> also
>> https://lists.nongnu.org/archive/html/qemu-devel/2006-06/msg00196.html
>>
>> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>
>> Message-Id: <1527105041-21013-1-git-send-email-patryk@fala.ehost.pl>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  chardev/char-serial.c | 2 +-
>>  chardev/char-stdio.c  | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/chardev/char-serial.c b/chardev/char-serial.c
>> index feb52e559d..ae548d28da 100644
>> --- a/chardev/char-serial.c
>> +++ b/chardev/char-serial.c
>> @@ -139,7 +139,7 @@ static void tty_serial_init(int fd, int speed,
>>  
>>      tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
>>                       | INLCR | IGNCR | ICRNL | IXON);
>> -    tty.c_oflag |= OPOST;
>> +    tty.c_oflag &= ~OPOST;
>>      tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN | ISIG);
>>      tty.c_cflag &= ~(CSIZE | PARENB | PARODD | CRTSCTS | CSTOPB);
>>      switch (data_bits) {
>> diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c
>> index 96375f2ab8..d83e60e787 100644
>> --- a/chardev/char-stdio.c
>> +++ b/chardev/char-stdio.c
>> @@ -59,7 +59,7 @@ static void qemu_chr_set_echo_stdio(Chardev *chr, bool echo)
>>      if (!echo) {
>>          tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
>>                           | INLCR | IGNCR | ICRNL | IXON);
>> -        tty.c_oflag |= OPOST;
>> +        tty.c_oflag &= ~OPOST;
>>          tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN);
>>          tty.c_cflag &= ~(CSIZE | PARENB);
>>          tty.c_cflag |= CS8;

The change to char-stdio.c wasn't actually introduced by me.
(https://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg05416.html).
Anyway, I haven't yet investigated it thoroughly but right now I think the problem is with that error reporting system. After all serial device shouldn't alter data coming from the guest. You never know when somebody will come up with crazy idea of pushing binary data through stdout.
Greg Kurz June 9, 2018, 7:31 a.m. UTC | #4 
On Fri, 8 Jun 2018 14:56:20 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 06/08/2018 02:39 PM, Greg Kurz wrote:
> > On Thu, 31 May 2018 19:16:05 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >   
> >> From: Patryk Olszewski <patryk@fala.ehost.pl>
> >>
> >> This patch fixes a bug in serial that made it almost impossible for guest
> >> to communicate with devices through host's serial.
> >>
> >> OPOST flag in c_oflag enables output processing letting other flags in
> >> c_oflag take effect. Usually in c_oflag ONLCR flag is also set, which
> >> causes crlf to be sent in place of lf. This breaks binary transmissions.
> >> Unsetting OPOST flag turns off any output processing which fixes the bug.
> >>  
> > 
> > But it damages error reporting...
> > 
> > Without this patch:
> > 
> > $ qemu-system-ppc64 -serial stdio -kernel foo
> > foo: No such file or directory
> > qemu-system-ppc64: error loading foo: Failed to load ELF
> > $
> > 
> > With this patch:
> > 
> > $ .mbuild-ppc-for-3.0/obj/ppc64-softmmu/qemu-system-ppc64 -serial stdio -kernel foo
> > foo: No such file or directory
> >                               qemu-system-ppc64: error loading foo: Failed to load ELF
> >                                                                                       $
> > 
> > It is possible to patch vreport() to append an explicit CR:
> > 
> >      error_vprintf(fmt, ap);
> > -    error_printf("\n");
> > +    error_printf("\n\r");
> >  }
> > 
> > but it only fixes the trailing newline of error_report(). Any other newline,
> > eg when using error_append_hint(), will lack the CR... Not sure how to fix
> > this :-\  
> 
> Peter just pushed the fix (ed6b018ef7):
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg02152.html
> 

Ah, cool ! :)

> >   
> >> Bug reports related:
> >> https://bugs.launchpad.net/qemu/+bug/1772086
> >> https://bugs.launchpad.net/qemu/+bug/1407813
> >> https://bugs.launchpad.net/qemu/+bug/1715296
> >> also
> >> https://lists.nongnu.org/archive/html/qemu-devel/2006-06/msg00196.html
> >>
> >> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>
> >> Message-Id: <1527105041-21013-1-git-send-email-patryk@fala.ehost.pl>
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >> Reviewed-by: Thomas Huth <thuth@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  chardev/char-serial.c | 2 +-
> >>  chardev/char-stdio.c  | 2 +-
> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/chardev/char-serial.c b/chardev/char-serial.c
> >> index feb52e559d..ae548d28da 100644
> >> --- a/chardev/char-serial.c
> >> +++ b/chardev/char-serial.c
> >> @@ -139,7 +139,7 @@ static void tty_serial_init(int fd, int speed,
> >>  
> >>      tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
> >>                       | INLCR | IGNCR | ICRNL | IXON);
> >> -    tty.c_oflag |= OPOST;
> >> +    tty.c_oflag &= ~OPOST;
> >>      tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN | ISIG);
> >>      tty.c_cflag &= ~(CSIZE | PARENB | PARODD | CRTSCTS | CSTOPB);
> >>      switch (data_bits) {
> >> diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c
> >> index 96375f2ab8..d83e60e787 100644
> >> --- a/chardev/char-stdio.c
> >> +++ b/chardev/char-stdio.c
> >> @@ -59,7 +59,7 @@ static void qemu_chr_set_echo_stdio(Chardev *chr, bool echo)
> >>      if (!echo) {
> >>          tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
> >>                           | INLCR | IGNCR | ICRNL | IXON);
> >> -        tty.c_oflag |= OPOST;
> >> +        tty.c_oflag &= ~OPOST;
> >>          tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN);
> >>          tty.c_cflag &= ~(CSIZE | PARENB);
> >>          tty.c_cflag |= CS8;  
> >
diff mbox series

Patch

diff --git a/chardev/char-serial.c b/chardev/char-serial.c
index feb52e559d..ae548d28da 100644
--- a/chardev/char-serial.c
+++ b/chardev/char-serial.c
@@ -139,7 +139,7 @@  static void tty_serial_init(int fd, int speed,
 
     tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
                      | INLCR | IGNCR | ICRNL | IXON);
-    tty.c_oflag |= OPOST;
+    tty.c_oflag &= ~OPOST;
     tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN | ISIG);
     tty.c_cflag &= ~(CSIZE | PARENB | PARODD | CRTSCTS | CSTOPB);
     switch (data_bits) {
diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c
index 96375f2ab8..d83e60e787 100644
--- a/chardev/char-stdio.c
+++ b/chardev/char-stdio.c
@@ -59,7 +59,7 @@  static void qemu_chr_set_echo_stdio(Chardev *chr, bool echo)
     if (!echo) {
         tty.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
                          | INLCR | IGNCR | ICRNL | IXON);
-        tty.c_oflag |= OPOST;
+        tty.c_oflag &= ~OPOST;
         tty.c_lflag &= ~(ECHO | ECHONL | ICANON | IEXTEN);
         tty.c_cflag &= ~(CSIZE | PARENB);
         tty.c_cflag |= CS8;