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

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

[Patryk Olszewski]

Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>
---
 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) {
-- 
2.7.4

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

[Markus Armbruster]

Patryk Olszewski <patryk@fala.ehost.pl> writes:

> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>
> ---
>  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."

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

[Peter Maydell]

On 22 May 2018 at 12:53, Markus Armbruster <armbru@redhat.com> wrote:
> Patryk Olszewski <patryk@fala.ehost.pl> writes:
>
>> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>
>> ---
>>  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

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

[Patryk]

W dniu 22.05.2018 o 15:07, Peter Maydell pisze:
> On 22 May 2018 at 12:53, Markus Armbruster <armbru@redhat.com> wrote:
>> Patryk Olszewski <patryk@fala.ehost.pl> writes:
>>
>>> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>
>>> ---
>>>  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

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

[Markus Armbruster]

Patryk <patryk@fala.ehost.pl> writes:

> W dniu 22.05.2018 o 15:07, Peter Maydell pisze:
>> On 22 May 2018 at 12:53, Markus Armbruster <armbru@redhat.com> wrote:
>>> Patryk Olszewski <patryk@fala.ehost.pl> writes:
>>>
>>>> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>
>>>> ---
>>>>  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 :)

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

[Patryk Olszewski]

W dniu 23.05.2018 o 18:40, Markus Armbruster pisze:
> Patryk <patryk@fala.ehost.pl> writes:
>
>> W dniu 22.05.2018 o 15:07, Peter Maydell pisze:
>>> On 22 May 2018 at 12:53, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Patryk Olszewski <patryk@fala.ehost.pl> writes:
>>>>
>>>>> Signed-off-by: Patryk Olszewski <patryk@fala.ehost.pl>
>>>>> ---
>>>>>  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 :)
>
First time contribution to such large project. Trying to learn how to
properly work with mailing lists and such. If there's something I
actually have to do now I'd like to get some pointers on how to do it.
Thank you,

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

[Eric Blake]

On 05/23/2018 11:48 AM, Patryk Olszewski wrote:

>>> 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 :)
>>
> First time contribution to such large project. Trying to learn how to
> properly work with mailing lists and such. If there's something I
> actually have to do now I'd like to get some pointers on how to do it.
> Thank you,

Then welcome to the qemu community!  In answer to your question, yes, 
now that you've had some valid reviewer comments, the ball is back in 
your court to incorporate the changes that were suggested to you, and 
submit a new top-level thread with a v2 in the subject line.

The easiest thing is to do 'git commit --amend' (works for a single 
commit) or even 'git rebase -i' then change the patch from 'pick' to 
'edit' (required when editing a patch series, but can also be used on a 
single patch to learn the patch flow), make the requested edits (that 
is, add your rationale into the commit message), and then send your new 
patch revision ('git send-email -1 -v2' or similar).  It also helps if 
version 2 contains a description after the '---' separator explaining 
how it is improved from v1 (in this case, improving the commit message). 
  Other patch submission hints can be found at 
http://wiki.qemu.org/Contribute/SubmitAPatch

Don't worry if you don't get everything perfect on your first try; we 
were all once beginners, so we don't mind helping someone else learn. 
But at the same time, demonstrating that you were able to make life 
easier for reviewers by following as many suggestions as possible, 
rather than leaving all the cleanup work to someone else, is an 
important skill to have if you plan to stick around for further 
contributions.

Also, feel free to ask questions on IRC where you might get faster 
response time, when it comes to figuring out your ideal setup for 
submitting patches with git.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org