chardev/char-serial.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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
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."
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
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
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 :)
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,
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
© 2016 - 2024 Red Hat, Inc.