Re: [Qemu-devel] [PATCH] Remove unwanted crlf conversion in serial

[Markus Armbruster]

Patryk <address@hidden> writes:

> W dniu 22.05.2018 o 15:07, Peter Maydell pisze:
>> On 22 May 2018 at 12:53, Markus Armbruster <address@hidden> wrote:
>>> Patryk Olszewski <address@hidden> writes:
>>>
>>>> Signed-off-by: Patryk Olszewski <address@hidden>
>>>> ---
>>>>  chardev/char-serial.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/chardev/char-serial.c b/chardev/char-serial.c
>>>> index feb52e5..ae548d2 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) {
>>> This change may well make sense, but your commit message needs to
>>> explain *why*.
>>>
>>> For what it's worth, POSIX documents OPOST as "Post-process output", and
>>> the Linux manual page as "Enable implementation-defined output
>>> processing."
>> We've set OPOST on our terminals since forever, right back to
>> commit 0824d6fc674 in 2003. Not setting it seems like the right thing,
>> though, since we're generally otherwise setting the thing up to be a
>> raw mode tty (and if we're connecting this to a guest then raw seems
>> like what we want).
>>
>> I wonder whether connecting, say, the HMP monitor to a 'serial'
>> chardev (a) works now (b) ends up with stair-step output after
>> this change (c) is something we care about...
>>
>> thanks
>> -- PMM
>>
> This patch is here to help fix years old bug of lf being replaced with
> crlf in serial, which is super problematic in binary transmissions,
> making communication with devices through serial from guest almost
> impossible.
>
> Setting OPOST flag in c_oflag enables the output processing, in other
> words it makes any other flag set in c_oflag come into action. From my
> quick experiment on serial devices on Linux I found out that by default
> c_oflag has enabled ONLCR flag, which is the one responsible for the
> crlf conversion. Unsetting OPOST disables it.
>
> Bug reports related to that:
>
> 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

Work that into your commit message, and you got a fine patch as far as
I'm concerned :)