Re: [Qemu-devel] [PULL 52/53] char: Remove unwanted crlf conversion

[Patryk Olszewski <patryk@fala.ehost.pl>]

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.