svn commit: r355301 - head/usr.sbin/bhyve

[Rodney W. Grimes]

> I see, thanks for the pointers.
> It looks like cfmakeraw() and tcsetattr() were what I was looking for.
> A bhyve-specific printf wrapper looks the right solution to me.
> I can try to sketch a patch for you guys to review, if that's useful.
> 
> Cheers,
>   Vincenzo

Meanwhile could you please revert the commit, and add a note to
D22552 to the effects that this was not the right solution?

Thanks,
Rod

> Il giorno mar 3 dic 2019 alle ore 18:38 John Baldwin <jhb at freebsd.org> ha
> scritto:
> 
> > On 12/3/19 7:14 AM, Ian Lepore wrote:
> > > On Mon, 2019-12-02 at 23:22 -0800, Rodney W. Grimes wrote:
> > >>> On Mon, 2019-12-02 at 20:51 +0000, Vincenzo Maffione wrote:
> > >>>> Author: vmaffione
> > >>>> Date: Mon Dec  2 20:51:46 2019
> > >>>> New Revision: 355301
> > >>>> URL: https://svnweb.freebsd.org/changeset/base/355301
> > >>>>
> > >>>> Log:
> > >>>>   bhyve: uniform printf format string newlines
> > >>>>
> > >>>>   Some of the printf statements only use LF to get a newline.
> > >>>> However, a CR character is also required for the serial console to
> > >>>> print debug logs in a nice way.
> > >>>>   Fix those code locations that only use LF, by adding a CR
> > >>>> character.
> > >>>>
> > >>>>   Reviewed by:     markj, aleksandr.fedorov at itglobal.com
> > >>>>   MFC after:       1 week
> > >>>>   Differential Revision:   https://reviews.freebsd.org/D22552
> > >>>>
> > >>>> Modified:
> > >>>>   head/usr.sbin/bhyve/audio.c
> > >>>>   head/usr.sbin/bhyve/hda_codec.c
> > >>>>   head/usr.sbin/bhyve/net_backends.c
> > >>>>   head/usr.sbin/bhyve/pci_ahci.c
> > >>>>   head/usr.sbin/bhyve/pci_e82545.c
> > >>>>   head/usr.sbin/bhyve/pci_hda.c
> > >>>>   head/usr.sbin/bhyve/pci_nvme.c
> > >>>>   head/usr.sbin/bhyve/pci_virtio_block.c
> > >>>>   head/usr.sbin/bhyve/pci_virtio_console.c
> > >>>>   head/usr.sbin/bhyve/pci_virtio_net.c
> > >>>>   head/usr.sbin/bhyve/pci_virtio_rnd.c
> > >>>>   head/usr.sbin/bhyve/pci_virtio_scsi.c
> > >>>>   head/usr.sbin/bhyve/pci_xhci.c
> > >>>>   head/usr.sbin/bhyve/rfb.c
> > >>>>
> > >>>
> > >>> These changes seem wrong in a couple ways...
> > >>>
> > >>>  - Lines are terminated by linefeeds in unix-like systems.  If
> > >>> linefeeds need to be translated to include carriage returns, that's the
> > >>> responsibility of the terminal/line-discipline layer, not the source
> > >>> strings being printed.
> > >>
> > >> Fully agree, this change seems wrong to me for Ian's stated reason here.
> > >>
> > >>>
> > >>>  - The sequence \n\r is very strange.  For systems that do prefer
> > >>> carriage returns, the \r always comes before the \n (or stands alone on
> > >>> Mac systems), not after.
> > >>>
> > >>> I have a feeling that the root of this is something like "lots of
> > >>> people use bhyve for Windows, so they use Windows apps to look at logs,
> > >>> so the logs should be formatted for Windows."  If that's the reasoning,
> > >>> then why shouldn't we convert EVERY printf in the source base to
> > >>> include carriage returns, just in case a windows user wants to browse a
> > >>> log file?
> > >>
> > >> This is not that issue, it is something going on with the line
> > >> discipline when using the bhyve console device.  I believe the
> > >> line displine being different from what bhyve itself is expecting
> > >> so when console output is intermixed with output from bhyve itself
> > >> things go wrong.
> > >>
> > >> The printf's in this patch are coming from the bhyve process that
> > >> has a fd open to the launching tty, the line discipline on that tty
> > >> is changed to something different after you open the
> > >> console device from that same controlling tty, or that is my hypothosis
> > >> on what is going wrong.
> > >
> > > There is a cfmakeraw() call in usr.sbin/bhyve/consport.c; that would
> > > definitely turn off nl->crnl translations.  I think that is the other
> > > end of the bhyve console that I posted a patch for yesterday, and I
> > > think the console driver is probably still the right place to do that
> > > translation (because other console drivers do it that way).  But I'm
> > > not set up to run bhyve here, so I can't test my theory.
> >
> > That patch won't work alone.  Most people don't use bvmcons, most people
> > running bhyve use a standard uart as the console (bvmcons was an early
> > console devices before bhyve had a ns8250 uart device model).  When using
> > the uart as the device model you still have raw output in the bhyve
> > process itself.  (See cfmakeraw() in uart_emul.c as well).  We don't
> > get to change how guest OS's use a uart, so any changes have to be in
> > usr.sbin/bhyve, not in sys/.
> >
> > However, to do that you have to actually do something more complicated to
> > turn \r\n and \n\r sequences from the guest into plain \n to stdout while
> > still DTRT for "bare" \r and \n characters.  You also have to make sure
> > you do the right thing for input and not just output in the device models.
> >
> > I'm not quite a fan of this commit as-is since you will get spurious new
> > lines now if you don't use a serial console.  I would perhaps rather have
> > a custom printf() wrapper in bhyve that outputs the \r as needed for
> > debug and error printfs only when stdio has been changed to be raw.  I
> > was busy with family stuff and thanksgiving last week so didn't have time
> > to review it before it was committed unfortunately.
> >
> > --
> > John Baldwin
> >

-- 
Rod Grimes                                                 rgrimes at freebsd.org