Re: [Minios-devel] [PATCH] mini-os: Remove \r\n conversion when writing to PV serial port

[Simon Waterman <watermansrdev@xxxxxxxxx>]

On 11/05/17 17:26, Wei Liu wrote:
> On Wed, May 10, 2017 at 11:33:23PM +0100, Simon Waterman wrote:
> > This change removes \n to \r\n conversion when writing to a PV serial
> > port so that binary protocols carried over the port are not corrupted.
> >
> > The change enables new use-cases for example connectivity from the Windows
> > Kernel Debugger to a guest via it's IOEMU stubdom.
> >
> > Signed-off-by: Simon Waterman <watermansrdev@xxxxxxxxx>
> > ---
> Actually, from a conversation on #xendevel:
>
> <Diziet> I think a serial port (and therefore by extension a console)
> contains something roughly like a terminal would expect (eg, something
> like a telnet NVT bytestream).
> <Diziet> All other UNIXish operating systems turn  printf("\n")  into
> \r\n.
> <Diziet> Normally they do this in the tty driver.
>
> <Diziet> Then if the guest sends just \n, you will get staircase output.
> <Diziet> Obviously the Xen PV console protocol can be used for other
> things besides being an actual console.
> <Diziet> But if that is desired, the line ending conversion should be
> moved to something related to consoleness in the guest (ie in minios),
> not just deleted.
>
> I think Ian explained why it was done like that.
>
> We should probably move this somewhere else.
>
> Simon, can you clarify your use case? Doesn't this patch cause xl
> console to print out staircase output?
Main use case is Windows kernel debugging but some peripherals using
serial comms don't like the conversion either.

The CRNL conversion is done by the guest in the IOEMU stubdom case
so no staircases.  In fact you get CRCRNL without the patch.

The staircase would happen if you call write() within minios itself
but in the IOEMU stubdom case this is dumped into the QEMU log file
where it doesn't matter.

Another unikernel with a proper console would be broken though and
I hadn't thought of that.  Makes sense now why the code was there in
the first place.

I still think it would be nice for IOEMU stubdoms and mini-os based
unikernels in general to support raw IO on the serial ports. Perhaps
I could add limited termios support to mini-os allowing the
user of the console to switch CRNL processing on and off (using ONLCR).

Qemu would be changed to turn this off in the stubdom case - assuming
what the guest sends is what should appear on the backend.  It could
be used by another unikernel that wants a raw serial port too.

> >   console/console.c | 29 +----------------------------
> >   1 file changed, 1 insertion(+), 28 deletions(-)
> >
> > diff --git a/console/console.c b/console/console.c
> > index 5538bd4..de94ad6 100644
> > --- a/console/console.c
> > +++ b/console/console.c
> > @@ -82,10 +82,6 @@ void xencons_tx(void)
> >   
> >   void console_print(struct consfront_dev *dev, char *data, int length)
> >   {
> > -    char *curr_char, saved_char;
> > -    char copied_str[length+1];
> > -    char *copied_ptr;
> > -    int part_len;
> >       int (*ring_send_fn)(struct consfront_dev *dev, const char *data, unsigned 
> > length);
> >   
> >       if(!console_initialised)
> > @@ -93,30 +89,7 @@ void console_print(struct consfront_dev *dev, char *data, 
> > int length)
> >       else
> >           ring_send_fn = xencons_ring_send;
> >   
> > -    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')
> > -        {
> > -            *curr_char = '\r';
> > -            saved_char = *(curr_char+1);
> > -            *(curr_char+1) = '\n';
> > -            part_len = curr_char - copied_ptr + 2;
> > -            ring_send_fn(dev, copied_ptr, part_len);
> > -            *(curr_char+1) = saved_char;
> > -            copied_ptr = curr_char+1;
> > -            length -= part_len - 1;
> > -        }
> > -    }
> > -
> > -    if (copied_ptr[length-1] == '\n') {
> > -        copied_ptr[length-1] = '\r';
> > -        copied_ptr[length] = '\n';
> > -        length++;
> > -    }
> > -
> > -    ring_send_fn(dev, copied_ptr, length);
> > +    ring_send_fn(dev, data, length);
> >   }
> >   
> >   void print(int direct, const char *fmt, va_list args)
> > --
> > 2.7.4

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/minios-devel