[PATCH 4 of 7] minios: fix minios console

[PATCH 4 of 7] minios: fix minios console

[Stefano Stabellini]

MiniOS' console_print tries to expand '\n' into "\n\r" in place, causing
page faults if the string resides in text.
Use a duplicate of the string instead.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

diff -r 21935dfa8e5f extras/mini-os/console/console.c
--- a/extras/mini-os/console/console.c	Fri Oct 09 19:30:39 2009 +0100
+++ b/extras/mini-os/console/console.c	Mon Oct 12 16:03:49 2009 +0100
@@ -79,6 +79,8 @@
 void console_print(struct consfront_dev *dev, char *data, int length)
 {
     char *curr_char, saved_char;
+    char copied_str[length];
+    char *copied_ptr;
     int part_len;
     int (*ring_send_fn)(struct consfront_dev *dev, const char *data, unsigned length);
 
@@ -86,24 +88,26 @@
         ring_send_fn = xencons_ring_send_no_notify;
     else
         ring_send_fn = xencons_ring_send;
-        
-    for(curr_char = data; curr_char < data+length-1; curr_char++)
+
+    copied_ptr = copied_str;
+    memcpy(copied_ptr, data, length);
+    for(curr_char = copied_ptr; curr_char < copied_ptr+length-1; curr_char++)
     {
         if(*curr_char == '\n')
         {
             saved_char = *(curr_char+1);
             *(curr_char+1) = '\r';
-            part_len = curr_char - data + 2;
-            ring_send_fn(dev, data, part_len);
+            part_len = curr_char - copied_ptr + 2;
+            ring_send_fn(dev, copied_ptr, part_len);
             *(curr_char+1) = saved_char;
-            data = curr_char+1;
+            copied_ptr = curr_char+1;
             length -= part_len - 1;
         }
     }
     
-    ring_send_fn(dev, data, length);
+    ring_send_fn(dev, copied_ptr, length);
     
-    if(data[length-1] == '\n')
+    if(copied_ptr[length-1] == '\n')
         ring_send_fn(dev, "\r", 1);
 }

Re: [PATCH 4 of 7] minios: fix minios console

[Mick Jordan]

On 10/12/09 10:20 AM, Stefano Stabellini wrote:
> MiniOS' console_print tries to expand '\n' into "\n\r" in place, causing
> page faults if the string resides in text.
> Use a duplicate of the string instead.
>   
Are you sure this is even necessary? I removed this in my GUK extension 
of MiniOS with no apparent effect. The actual push to remove it was 
observing that when "piping" the output of one GUK (JVM) guest to 
another, the "\n\r" was being treated as two line feeds by the Java 
runtime. Why? Because it is "\r\n" that is historically reduced to "\n", 
and not "\n\r". In any event in the Xen/Unix environment there should be 
no need for the expansion at all. And it really helped to clean up the 
console code.

Mick

Re: [PATCH 4 of 7] minios: fix minios console

[Stefano Stabellini]

On Mon, 12 Oct 2009, Mick Jordan wrote:
> On 10/12/09 10:20 AM, Stefano Stabellini wrote:
> > MiniOS' console_print tries to expand '\n' into "\n\r" in place, causing
> > page faults if the string resides in text.
> > Use a duplicate of the string instead.
> >   
> Are you sure this is even necessary? I removed this in my GUK extension 
> of MiniOS with no apparent effect. The actual push to remove it was 
> observing that when "piping" the output of one GUK (JVM) guest to 
> another, the "\n\r" was being treated as two line feeds by the Java 
> runtime. Why? Because it is "\r\n" that is historically reduced to "\n", 
> and not "\n\r". In any event in the Xen/Unix environment there should be 
> no need for the expansion at all. And it really helped to clean up the 
> console code.
> 

The expansion of \n in \r\n is for consistency with linux ttys, but you
are right about the fact that minios actually substitutes \n with \n\r,
and this is definitely another bug.

Re: [PATCH 4 of 7] minios: fix minios console

[Mick Jordan]

On 10/13/09 05:22 AM, Stefano Stabellini wrote:
> The expansion of \n in \r\n is for consistency with linux ttys, but you
> are right about the fact that minios actually substitutes \n with \n\r,
> and this is definitely another bug.
>
>   
So what does  "consistency with linux ttys" actually mean in practice? I 
am running under Linux and am not seeing any issues without the 
conversion. Can you cite some situation where not doing the conversion 
would cause a problem? E.g., failing to start printing at the beginning 
of the line. [I assume no-one is using ASR-33's any more!].

Mick

Re: [PATCH 4 of 7] minios: fix minios console

[Ian Jackson]

Mick Jordan writes ("Re: [Xen-devel] [PATCH 4 of 7] minios: fix minios console"):
> So what does  "consistency with linux ttys" actually mean in practice? I 
> am running under Linux and am not seeing any issues without the 
> conversion. Can you cite some situation where not doing the conversion 
> would cause a problem? E.g., failing to start printing at the beginning 
> of the line. [I assume no-one is using ASR-33's any more!].

Well, it's like this: the Linux hvc driver (or its equivalent in
2.6.18 vintage kernels) gets the data after it has been processed by
the general tty layer.  In most cases (if the terminal is in the most
normal mode) this means that \n written by applications has been
already changed to \r\n.  This is because ttys are (virtual or real)
serial ports.

Thus, in current practice with non-stubdom, the data over the Xen
console ring is \r\n (and generally formatted as data you might send
over a serial port).  Thus the xen pv console is `supposed' to work
more like a virtual hardware serial line than like (say) a Unix pipe.

This is not dealt with wholly consistently by the xen-unstable
toolstack.  For example the default behaviour of xenconsoled is to
stuff the received data directly into a file; this is in some sense
wrong because arguably the file ought to be a Unix text file (that
is, a logfile) rather than a session transcript (as you might get out
of `script').

But if you were to plumb the console directly to a hardware serial
port or to another tty (eg one connected to a terminal emulator
program), this lf => crlf conversion is essential.  Without it you
will get incorrect output on whatever is displaying the stream.

Without a proper spec for all of this it's hard to say that anything
in particular is right or wrong but since the guest behaviour is
hardest to change I would suggest we declare that to be correct.  And
if we do that, then we must also say that minios should behave like
other guests and write cr-lf terminated lines, not lf-terminated ones.

Ian.