Message ID | 20171120071423.11693-1-david@gibson.dropbear.id.au (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH] spapr: Implement bug in spapr-vty device to be compatible with PowerVM Type: series Message-id: 20171120071423.11693-1-david@gibson.dropbear.id.au === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20171120071423.11693-1-david@gibson.dropbear.id.au -> patchew/20171120071423.11693-1-david@gibson.dropbear.id.au Switched to a new branch 'test' 08e9d8fe82 spapr: Implement bug in spapr-vty device to be compatible with PowerVM === OUTPUT BEGIN === Checking PATCH 1/1: spapr: Implement bug in spapr-vty device to be compatible with PowerVM... ERROR: spaces required around that '-' (ctx:VxV) #40: FILE: hw/char/spapr_vty.c:68: + if (buf[n-1] == '\r') { ^ total: 1 errors, 0 warnings, 24 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org
On Sun, Nov 19, 2017 at 11:18:55PM -0800, no-reply@patchew.org wrote: > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > Subject: [Qemu-devel] [PATCH] spapr: Implement bug in spapr-vty device to be compatible with PowerVM > Type: series > Message-id: 20171120071423.11693-1-david@gibson.dropbear.id.au > > === TEST SCRIPT BEGIN === > #!/bin/bash > > BASE=base > n=1 > total=$(git log --oneline $BASE.. | wc -l) > failed=0 > > git config --local diff.renamelimit 0 > git config --local diff.renames True > > commits="$(git log --format=%H --reverse $BASE..)" > for c in $commits; do > echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." > if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then > failed=1 > echo > fi > n=$((n+1)) > done > > exit $failed > === TEST SCRIPT END === > > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 > From https://github.com/patchew-project/qemu > * [new tag] patchew/20171120071423.11693-1-david@gibson.dropbear.id.au -> patchew/20171120071423.11693-1-david@gibson.dropbear.id.au > Switched to a new branch 'test' > 08e9d8fe82 spapr: Implement bug in spapr-vty device to be compatible with PowerVM > > === OUTPUT BEGIN === > Checking PATCH 1/1: spapr: Implement bug in spapr-vty device to be compatible with PowerVM... > ERROR: spaces required around that '-' (ctx:VxV) > #40: FILE: hw/char/spapr_vty.c:68: > + if (buf[n-1] == '\r') { I've corrected this in my tree.
On 20.11.2017 08:14, David Gibson wrote: > The spapr-vty device implements the PAPR defined virtual console, > which is also implemented by IBM's proprietary PowerVM hypervisor. > > PowerVM's implementation has a bug where it inserts an extra \0 after > every \r going to the guest. Because of that Linux's guest side > driver has a workaround which strips \0 characters that appear > immediately after a \r. Ouch. I wonder whether it would make more sense to change the guest side driver to apply the workaround only if it's really running under PowerVM...? E.g. what if the PowerVM bug ever gets fixed one day? > That means that when running under qemu, sending a binary stream from > host to guest via spapr-vty which happens to include a \r\0 sequence > will get corrupted by that workaround. > > To deal with that, this patch duplicates PowerVM's bug, inserting an > extra \0 after each \r. Ugly, but the best option available. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/char/spapr_vty.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c > index 0fa416ca6b..a95e5e91a7 100644 > --- a/hw/char/spapr_vty.c > +++ b/hw/char/spapr_vty.c > @@ -58,6 +58,24 @@ static int vty_getchars(VIOsPAPRDevice *sdev, uint8_t *buf, int max) > > while ((n < max) && (dev->out != dev->in)) { > buf[n++] = dev->buf[dev->out++ % VTERM_BUFSIZE]; > + > + /* PowerVM's vty implementation has a bug where it inserts a > + * \0 after every \r going to the guest. Existing guests have > + * a workaround for this which removes every \0 immediately > + * following a \r, so here we make ourselves bug-for-bug > + * compatible, so that the guest won't drop a real \0-after-\r > + * that happens to occur in a binary stream. */ > + if (buf[n-1] == '\r') { > + if (n < max) { > + buf[n++] = '\0'; > + } else { > + /* No room for the extra \0, roll back and try again > + * next time */ > + dev->out--; > + n--; > + break; > + } > + } > } > > qemu_chr_fe_accept_input(&dev->chardev); > Code looks fine to me, so: Reviewed-by: Thomas Huth <thuth@redhat.com>
On Tue, 21 Nov 2017 08:27:51 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 20.11.2017 08:14, David Gibson wrote: > > The spapr-vty device implements the PAPR defined virtual console, > > which is also implemented by IBM's proprietary PowerVM hypervisor. > > > > PowerVM's implementation has a bug where it inserts an extra \0 after > > every \r going to the guest. Because of that Linux's guest side > > driver has a workaround which strips \0 characters that appear > > immediately after a \r. > > Ouch. > > I wonder whether it would make more sense to change the guest side > driver to apply the workaround only if it's really running under > PowerVM...? E.g. what if the PowerVM bug ever gets fixed one day? > The bug has been around forever (already there in initial linux git commit 1da177e4c3f4), so I'm not sure PowerVM will ever fix it. It is legacy API now :) Also I'm not sure it is worth the pain to introduce extra complexity in the existing linux driver... I guess we'd rather encourage people to switch to virtio serial instead. Anyway, Reviewed-by: Greg Kurz <groug@kaod.org> > > That means that when running under qemu, sending a binary stream from > > host to guest via spapr-vty which happens to include a \r\0 sequence > > will get corrupted by that workaround. > > > > To deal with that, this patch duplicates PowerVM's bug, inserting an > > extra \0 after each \r. Ugly, but the best option available. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/char/spapr_vty.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c > > index 0fa416ca6b..a95e5e91a7 100644 > > --- a/hw/char/spapr_vty.c > > +++ b/hw/char/spapr_vty.c > > @@ -58,6 +58,24 @@ static int vty_getchars(VIOsPAPRDevice *sdev, uint8_t *buf, int max) > > > > while ((n < max) && (dev->out != dev->in)) { > > buf[n++] = dev->buf[dev->out++ % VTERM_BUFSIZE]; > > + > > + /* PowerVM's vty implementation has a bug where it inserts a > > + * \0 after every \r going to the guest. Existing guests have > > + * a workaround for this which removes every \0 immediately > > + * following a \r, so here we make ourselves bug-for-bug > > + * compatible, so that the guest won't drop a real \0-after-\r > > + * that happens to occur in a binary stream. */ > > + if (buf[n-1] == '\r') { > > + if (n < max) { > > + buf[n++] = '\0'; > > + } else { > > + /* No room for the extra \0, roll back and try again > > + * next time */ > > + dev->out--; > > + n--; > > + break; > > + } > > + } > > } > > > > qemu_chr_fe_accept_input(&dev->chardev); > > > > Code looks fine to me, so: > > Reviewed-by: Thomas Huth <thuth@redhat.com> >
diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c index 0fa416ca6b..a95e5e91a7 100644 --- a/hw/char/spapr_vty.c +++ b/hw/char/spapr_vty.c @@ -58,6 +58,24 @@ static int vty_getchars(VIOsPAPRDevice *sdev, uint8_t *buf, int max) while ((n < max) && (dev->out != dev->in)) { buf[n++] = dev->buf[dev->out++ % VTERM_BUFSIZE]; + + /* PowerVM's vty implementation has a bug where it inserts a + * \0 after every \r going to the guest. Existing guests have + * a workaround for this which removes every \0 immediately + * following a \r, so here we make ourselves bug-for-bug + * compatible, so that the guest won't drop a real \0-after-\r + * that happens to occur in a binary stream. */ + if (buf[n-1] == '\r') { + if (n < max) { + buf[n++] = '\0'; + } else { + /* No room for the extra \0, roll back and try again + * next time */ + dev->out--; + n--; + break; + } + } } qemu_chr_fe_accept_input(&dev->chardev);
The spapr-vty device implements the PAPR defined virtual console, which is also implemented by IBM's proprietary PowerVM hypervisor. PowerVM's implementation has a bug where it inserts an extra \0 after every \r going to the guest. Because of that Linux's guest side driver has a workaround which strips \0 characters that appear immediately after a \r. That means that when running under qemu, sending a binary stream from host to guest via spapr-vty which happens to include a \r\0 sequence will get corrupted by that workaround. To deal with that, this patch duplicates PowerVM's bug, inserting an extra \0 after each \r. Ugly, but the best option available. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/char/spapr_vty.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)