[2/2] s390x/3270: handle writes of arbitrary length

Commit Message

Halil Pasic Sept. 20, 2017, 5:23 p.m. UTC

The problem is, that the current implementation places unrealistic and
arbitrary constraints on the length of writes to the device (that is the
outbound requests), by asserting ccw.count being such that that even the
worst case escaped payload will fit an  more or less arbitrary sized
buffer. Actually on protocol level there is nothing to justify such
a limitation.

Another strange thing is the return value which more or less reflects
the size (written) after escaping instead of before escaping. This
is strange, because this return value is used to calculate SCSW.count.

Let us teach 3270 how to deal with arbitrary long writes.

Signed-off-by: Halil Pasic <[e-mail]>
Acked-by: Christian Borntraeger <[e-mail]>
Reviewed-by: Dong Jia Shi <[e-mail]>
Reported-by: Jason J . Herne <[e-mail]>
Tested-by: Jason J . Herne <[e-mail]>
---
 hw/char/terminal3270.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

Comments

Cornelia Huck Sept. 21, 2017, 9:23 a.m. UTC | #1

On Wed, 20 Sep 2017 19:23:14 +0200
Halil Pasic <[e-mail]> wrote:

> The problem is, that the current implementation places unrealistic and
> arbitrary constraints on the length of writes to the device (that is the
> outbound requests), by asserting ccw.count being such that that even the
> worst case escaped payload will fit an  more or less arbitrary sized
> buffer. Actually on protocol level there is nothing to justify such
> a limitation.
> 
> Another strange thing is the return value which more or less reflects
> the size (written) after escaping instead of before escaping. This
> is strange, because this return value is used to calculate SCSW.count.

Didn't the Linux driver care about the count?

> 
> Let us teach 3270 how to deal with arbitrary long writes.
> 
> Signed-off-by: Halil Pasic <[e-mail]>
> Acked-by: Christian Borntraeger <[e-mail]>
> Reviewed-by: Dong Jia Shi <[e-mail]>
> Reported-by: Jason J . Herne <[e-mail]>
> Tested-by: Jason J . Herne <[e-mail]>
> ---
>  hw/char/terminal3270.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)

Looks good.

Halil Pasic Sept. 21, 2017, 10:30 a.m. UTC | #2

On 09/21/2017 11:23 AM, Cornelia Huck wrote:
> On Wed, 20 Sep 2017 19:23:14 +0200
> Halil Pasic <[e-mail]> wrote:
> 
>> The problem is, that the current implementation places unrealistic and
>> arbitrary constraints on the length of writes to the device (that is the
>> outbound requests), by asserting ccw.count being such that that even the
>> worst case escaped payload will fit an  more or less arbitrary sized
>> buffer. Actually on protocol level there is nothing to justify such
>> a limitation.
>>
>> Another strange thing is the return value which more or less reflects
>> the size (written) after escaping instead of before escaping. This
>> is strange, because this return value is used to calculate SCSW.count.
> 
> Didn't the Linux driver care about the count?
> 

Maybe Jason can answer that. I did only most basic testing with my patch
applied (and basically no testing without my changes).

From code perspective I'm sure it does for the reads. For the writes
I did not look into that.

Halil

>>
>> Let us teach 3270 how to deal with arbitrary long writes.
>>
>> Signed-off-by: Halil Pasic <[e-mail]>
>> Acked-by: Christian Borntraeger <[e-mail]>
>> Reviewed-by: Dong Jia Shi <[e-mail]>
>> Reported-by: Jason J . Herne <[e-mail]>
>> Tested-by: Jason J . Herne <[e-mail]>
>> ---
>>  hw/char/terminal3270.c | 30 ++++++++++++++++++------------
>>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> Looks good.
>

Patch

diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index c976a63cc2..a109ce5987 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -30,7 +30,6 @@  typedef struct Terminal3270 {
     uint8_t inv[INPUT_BUFFER_SIZE];
     uint8_t outv[OUTPUT_BUFFER_SIZE];
     int in_len;
-    int out_len;
     bool handshake_done;
     guint timer_tag;
 } Terminal3270;
@@ -145,7 +144,6 @@  static void chr_event(void *opaque, int event)
 
     /* Ensure the initial status correct, always reset them. */
     t->in_len = 0;
-    t->out_len = 0;
     t->handshake_done = false;
     if (t->timer_tag) {
         g_source_remove(t->timer_tag);
@@ -231,8 +229,9 @@  static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd)
     Terminal3270 *t = TERMINAL_3270(dev);
     int retval = 0;
     int count = ccw_dstream_avail(get_cds(t));
-
-    assert(count <= (OUTPUT_BUFFER_SIZE - 3) / 2);
+    int bound = (OUTPUT_BUFFER_SIZE - 3) / 2;
+    int len = MIN(count, bound);
+    int out_len = 0;
 
     if (!t->handshake_done) {
         if (!(t->outv[0] == IAC && t->outv[1] != IAC)) {
@@ -247,16 +246,23 @@  static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd)
         /* We just say we consumed all data if there's no backend. */
         return count;
     }
-    t->outv[0] = cmd;
-    ccw_dstream_read_buf(get_cds(t), &t->outv[1], count);
-    t->out_len = count + 1;
 
-    t->out_len = insert_IAC_escape_char(t->outv, t->out_len);
-    t->outv[t->out_len++] = IAC;
-    t->outv[t->out_len++] = IAC_EOR;
+    t->outv[out_len++] = cmd;
+    do {
+        ccw_dstream_read_buf(get_cds(t), &t->outv[out_len], len);
+        count = ccw_dstream_avail(get_cds(t));
+        out_len += len;
 
-    retval = qemu_chr_fe_write_all(&t->chr, t->outv, t->out_len);
-    return (retval <= 0) ? 0 : (retval - 3);
+        out_len = insert_IAC_escape_char(t->outv, out_len);
+        if (!count) {
+            t->outv[out_len++] = IAC;
+            t->outv[out_len++] = IAC_EOR;
+        }
+        retval = qemu_chr_fe_write_all(&t->chr, t->outv, out_len);
+        len = MIN(count, bound);
+        out_len = 0;
+    } while (len && retval >= 0);
+    return (retval <= 0) ? 0 : get_cds(t)->count;
 }
 
 static Property terminal_properties[] = {