| Message ID | 1469791063-28222-1-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jul 29, 2016 at 12:17:43PM +0100, Daniel P. Berrange wrote: > The virtio-console.c file handles both serial consoles > and interactive consoles, since they're backed by the > same device model. > > Since serial devices are expected to be reliable and > need to notify the guest when the backend is opened > or closed, the virtio-console.c file wires up support > for chardev events. This affects both serial consoles > and interactive consoles, using a network connection > based chardev backend such as 'socket', but not when > using a PTY based backend or plain 'file' backends. > > When a device is open, however, the behaviour is > different - if the backend chardev returns EAGAIN or > a short write, the serial console will block and > setup a watch to poll for writability, ensuring no > data is lost. The interactive consoles meanwhile > will simply discard data. > > This means that the interactive consoles have different > blocking behaviour depending on whether the chardev is > open or not. If open, data may be discarded if not > consumed, where as if closed, data will always be queued > pending an open. > > This behaviour is unhelpful in general - applications > outputting messages on the guest console should not be > blocked simply because no client is conencted to the host > side. > > Consider for example, configuring a x86_64 guest with a > plain UART serial port > > -chardev socket,id=charserial1,host=127.0.0.1,port=9001,server,nowait,logfile=console1.log,logappend=on > -device isa-serial,chardev=charserial1,id=serial1 > > vs a s390 guest which has to use the virtio-console port > > -chardev socket,id=charconsole1,host=127.0.0.1,port=9000,server,nowait,logfile=console2.log,logappend=on > -device virtconsole,chardev=charconsole1,id=console1 > > The isa-serial one gets data written to the log regardless > of whether a client is connected, while the virtioconsole > one only gets data written to the log when a client is > connected. > > This patch changes the behaviour so that virtconsole > devices work in same way as other traditional console > devices. Specifically, the frontend will now be marked > as permanently open, so data flows regardless of the > backend status. > > NB, the behaviour of virtserialport devices is *not* > changed, only virtconsole. > > Fixes bug: https://bugs.launchpad.net/qemu/+bug/1599214 > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > hw/char/virtio-console.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > NB If there is a concern about backends compatibility with > this change, we could instead add a boolean property to > the virtio-console device 'explicit-open' which controls > whether the virtconsole device has the old behaviour or > the new behaviour and default to old. Personally I think > it is fine to just change behaviour for virtconsole > unconditionally though Ignore this patch for now. Looking around the users of chardev code I've noticed more inconsistency. So I want to analyse the broad usage and report on semantics across all, before proposing potential multiple fixes. Regards, Daniel
diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c index 2e36481..4f0e03d 100644 --- a/hw/char/virtio-console.c +++ b/hw/char/virtio-console.c @@ -85,8 +85,9 @@ static void set_guest_connected(VirtIOSerialPort *port, int guest_connected) { VirtConsole *vcon = VIRTIO_CONSOLE(port); DeviceState *dev = DEVICE(port); + VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port); - if (vcon->chr) { + if (vcon->chr && !k->is_console) { qemu_chr_fe_set_open(vcon->chr, guest_connected); } @@ -156,9 +157,25 @@ static void virtconsole_realize(DeviceState *dev, Error **errp) } if (vcon->chr) { - vcon->chr->explicit_fe_open = 1; - qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event, - vcon); + /* + * For consoles we don't block guest data transfer just + * because nothing is connected - we'll just let it go + * whetherever the chardev wants - /dev/null probably. + * + * For serial ports we need 100% reliable data transfer + * so we use the opened/closed signals from chardev to + * trigger open/close of the device + */ + if (k->is_console) { + vcon->chr->explicit_fe_open = 0; + qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, + NULL, vcon); + virtio_serial_open(port); + } else { + vcon->chr->explicit_fe_open = 1; + qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, + chr_event, vcon); + } } }
The virtio-console.c file handles both serial consoles and interactive consoles, since they're backed by the same device model. Since serial devices are expected to be reliable and need to notify the guest when the backend is opened or closed, the virtio-console.c file wires up support for chardev events. This affects both serial consoles and interactive consoles, using a network connection based chardev backend such as 'socket', but not when using a PTY based backend or plain 'file' backends. When a device is open, however, the behaviour is different - if the backend chardev returns EAGAIN or a short write, the serial console will block and setup a watch to poll for writability, ensuring no data is lost. The interactive consoles meanwhile will simply discard data. This means that the interactive consoles have different blocking behaviour depending on whether the chardev is open or not. If open, data may be discarded if not consumed, where as if closed, data will always be queued pending an open. This behaviour is unhelpful in general - applications outputting messages on the guest console should not be blocked simply because no client is conencted to the host side. Consider for example, configuring a x86_64 guest with a plain UART serial port -chardev socket,id=charserial1,host=127.0.0.1,port=9001,server,nowait,logfile=console1.log,logappend=on -device isa-serial,chardev=charserial1,id=serial1 vs a s390 guest which has to use the virtio-console port -chardev socket,id=charconsole1,host=127.0.0.1,port=9000,server,nowait,logfile=console2.log,logappend=on -device virtconsole,chardev=charconsole1,id=console1 The isa-serial one gets data written to the log regardless of whether a client is connected, while the virtioconsole one only gets data written to the log when a client is connected. This patch changes the behaviour so that virtconsole devices work in same way as other traditional console devices. Specifically, the frontend will now be marked as permanently open, so data flows regardless of the backend status. NB, the behaviour of virtserialport devices is *not* changed, only virtconsole. Fixes bug: https://bugs.launchpad.net/qemu/+bug/1599214 Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- hw/char/virtio-console.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) NB If there is a concern about backends compatibility with this change, we could instead add a boolean property to the virtio-console device 'explicit-open' which controls whether the virtconsole device has the old behaviour or the new behaviour and default to old. Personally I think it is fine to just change behaviour for virtconsole unconditionally though