Re: [PATCH v2 0/6] virtio-console: notify about the terminal size

[Michael S. Tsirkin]

On Thu, Jun 25, 2020 at 02:23:28PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 25, 2020 at 09:18:51AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Jun 24, 2020 at 12:49:15PM +0100, Daniel P. Berrangé wrote:
> > > On Wed, Jun 24, 2020 at 01:26:34PM +0200, Szymon Lukasz wrote:
> > > > Also there is a problem with the virtio spec and Linux Kernel
> > > > implementation, the order of fields in virtio_console_resize struct
> > > > differs between the kernel and the spec. I do not know if there is any
> > > > implementation of the virtio-console driver that handles resize messages
> > > > and uses a different order than Linux.
> > > 
> > > Well this is a bit of a mess :-(
> > > 
> > > The main virtio_console_config struct has cols, then rows.
> > > 
> > > The Linux impl of resizing appears to have arrived in 2010, and created
> > > a new struct with rows, then cols.
> > > 
> > > commit 8345adbf96fc1bde7d9846aadbe5af9b2ae90882
> > > Author: Amit Shah <amit.shah@redhat.com>
> > > Date:   Thu May 6 02:05:09 2010 +0530
> > > 
> > >     virtio: console: Accept console size along with resize control message
> > >     
> > >     The VIRTIO_CONSOLE_RESIZE control message sent to us by the host now
> > >     contains the new {rows, cols} values for the console. This ensures 
> > > each
> > >     console port gets its own size, and we don't depend on the 
> > > config-space
> > >     rows and cols values at all now.
> > >     
> > >     Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > >     CC: Christian Borntraeger <borntraeger@de.ibm.com>
> > >     CC: linuxppc-dev@ozlabs.org
> > >     CC: Kusanagi Kouichi <slash@ac.auone-net.jp>
> > >     Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > > 
> > > 
> > > The virtio spec documenting this came 4 years later in 2014 and documented
> > > the resize struct with cols, then rows, which differs from Linux impl,
> > > but matches ordering of the main virtio_console_config:
> > > 
> > > commit 908cfaa782e950d6656d947599d7a6c9fb16cad1
> > > Author: rusty <rusty@0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652>
> > > Date:   Wed Feb 12 03:15:57 2014 +0000
> > > 
> > >     Feedback #6: Applied
> > >     
> > >     As per minutes:
> > >             
> > > https://lists.oasis-open.org/archives/virtio/201402/msg00121.html
> > >     
> > >     Signed-off-by: Rusty Russell <rusty@au1.ibm.com>
> > >     
> > >     git-svn-id: 
> > > https://tools.oasis-open.org/version-control/svn/virtio@237 
> > > 0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652
> > > 
> > > I can understand why it is desirable for the resize struct to match
> > > the order of the initial config struct.  I'm guessing it just wasn't
> > > realized that the Linux impl was inverted for resize
> > > 
> > > The FreeBSD impl of virtio-console doesn't do resize:
> > > 
> > >   
> > > https://github.com/freebsd/freebsd/blob/master/sys/dev/virtio/console/virtio_console.c#L874
> > > 
> > > Not sure what other impls are going to be around, but I feel like
> > > Linux is going to be the most commonly deployed by orders of magnitude.
> > > 
> > > So I'd say QEMU should match Linux, and the spec should be fixed.
> > > 
> > > 
> > > Have you reported this bug to the virtio spec people directly yet ?
> > > 
> > > I don't see an issue open at
> > > 
> > >   https://github.com/oasis-tcs/virtio-spec/issues/
> > > 
> > > so I think one should be filed there
> > > 
> > > Regards,
> > > Daniel
> > 
> > 
> > One reports defects on the virtio-comments mailing list, issue tracker is 
> > just for
> > tracking spec changes.
> 
> NB That contradicts what the CONTRIBUTING.md file in virtio-spec says, which
> welcomes use of the issue tracker:
> 
>    "Persons who are not TC members are invited to open issues and
>     provide comments using this repository's GitHub Issues tracking
>     facility or using the TC's comment list. "
> 
> https://github.com/oasis-tcs/virtio-spec/blob/master/CONTRIBUTING.md

I know, it's confusing.  See

https://github.com/oasis-tcs/virtio-spec#use-of-github-issues

OASIS admins asked us not to change CONTRIBUTING.md so we
had to resort to adding clarifications in a separate file like that.



> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|