From owner-svn-src-head@freebsd.org Tue Dec 3 17:38:47 2019 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 0E8311B70F0; Tue, 3 Dec 2019 17:38:47 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 47S8NQ6hmrz3CHn; Tue, 3 Dec 2019 17:38:46 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from John-Baldwins-MacBook-Pro-5.local (ralph.baldwin.cx [66.234.199.215]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 0E9A51F59D; Tue, 3 Dec 2019 17:38:45 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Subject: Re: svn commit: r355301 - head/usr.sbin/bhyve To: Ian Lepore , rgrimes@freebsd.org Cc: Vincenzo Maffione , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201912030722.xB37MdrZ033595@gndrsh.dnsmgr.net> <8044a2f1096df626368183dd1ae77f5ac2e43b70.camel@freebsd.org> From: John Baldwin Openpgp: preference=signencrypt Autocrypt: addr=jhb@FreeBSD.org; keydata= mQGiBETQ+XcRBADMFybiq69u+fJRy/0wzqTNS8jFfWaBTs5/OfcV7wWezVmf9sgwn8TW0Dk0 c9MBl0pz+H01dA2ZSGZ5fXlmFIsee1WEzqeJzpiwd/pejPgSzXB9ijbLHZ2/E0jhGBcVy5Yo /Tw5+U/+laeYKu2xb0XPvM0zMNls1ah5OnP9a6Ql6wCgupaoMySb7DXm2LHD1Z9jTsHcAQMD /1jzh2BoHriy/Q2s4KzzjVp/mQO5DSm2z14BvbQRcXU48oAosHA1u3Wrov6LfPY+0U1tG47X 1BGfnQH+rNAaH0livoSBQ0IPI/8WfIW7ub4qV6HYwWKVqkDkqwcpmGNDbz3gfaDht6nsie5Z pcuCcul4M9CW7Md6zzyvktjnbz61BADGDCopfZC4of0Z3Ka0u8Wik6UJOuqShBt1WcFS8ya1 oB4rc4tXfSHyMF63aPUBMxHR5DXeH+EO2edoSwViDMqWk1jTnYza51rbGY+pebLQOVOxAY7k do5Ordl3wklBPMVEPWoZ61SdbcjhHVwaC5zfiskcxj5wwXd2E9qYlBqRg7QeSm9obiBCYWxk d2luIDxqaGJARnJlZUJTRC5vcmc+iGAEExECACAFAkTQ+awCGwMGCwkIBwMCBBUCCAMEFgID AQIeAQIXgAAKCRBy3lIGd+N/BI6RAJ9S97fvbME+3hxzE3JUyUZ6vTewDACdE1stFuSfqMvM jomvZdYxIYyTUpC5Ag0ERND5ghAIAPwsO0B7BL+bz8sLlLoQktGxXwXQfS5cInvL17Dsgnr3 1AKa94j9EnXQyPEj7u0d+LmEe6CGEGDh1OcGFTMVrof2ZzkSy4+FkZwMKJpTiqeaShMh+Goj XlwIMDxyADYvBIg3eN5YdFKaPQpfgSqhT+7El7w+wSZZD8pPQuLAnie5iz9C8iKy4/cMSOrH YUK/tO+Nhw8Jjlw94Ik0T80iEhI2t+XBVjwdfjbq3HrJ0ehqdBwukyeJRYKmbn298KOFQVHO EVbHA4rF/37jzaMadK43FgJ0SAhPPF5l4l89z5oPu0b/+5e2inA3b8J3iGZxywjM+Csq1tqz hltEc7Q+E08AAwUIAL+15XH8bPbjNJdVyg2CMl10JNW2wWg2Q6qdljeaRqeR6zFus7EZTwtX sNzs5bP8y51PSUDJbeiy2RNCNKWFMndM22TZnk3GNG45nQd4OwYK0RZVrikalmJY5Q6m7Z16 4yrZgIXFdKj2t8F+x613/SJW1lIr9/bDp4U9tw0V1g3l2dFtD3p3ZrQ3hpoDtoK70ioIAjjH aIXIAcm3FGZFXy503DOA0KaTWwvOVdYCFLm3zWuSOmrX/GsEc7ovasOWwjPn878qVjbUKWwx Q4QkF4OhUV9zPtf9tDSAZ3x7QSwoKbCoRCZ/xbyTUPyQ1VvNy/mYrBcYlzHodsaqUDjHuW+I SQQYEQIACQUCRND5ggIbDAAKCRBy3lIGd+N/BCO8AJ9j1dWVQWxw/YdTbEyrRKOY8YZNwwCf afMAg8QvmOWnHx3wl8WslCaXaE8= Message-ID: Date: Tue, 3 Dec 2019 09:38:42 -0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:60.0) Gecko/20100101 Thunderbird/60.9.1 MIME-Version: 1.0 In-Reply-To: <8044a2f1096df626368183dd1ae77f5ac2e43b70.camel@freebsd.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Dec 2019 17:38:47 -0000 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@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