On (Fri) 17 Jun 2011 [15:58:08], Luiz Capitulino wrote:
> If I start qemu with:
>
> # qemu -hda disks/test.img -enable-kvm -m 1G -snapshot \
> -device virtio-serial \
> -chardev socket,host=localhost,port=1234,server,nowait,id=foo \
> -device virtserialport,chardev=foo,name=org.qemu.guest_agent
>
> I get a segfault when booting a Fedora 14 guest. The backtrace says:
>
> Program terminated with signal 11, Segmentation fault.
> #0 0x0000000000420850 in handle_control_message (vser=0x3732bd0,
> buf=0x2c173e0, len=8) at
> /home/lcapitulino/src/qmp-unstable/hw/virtio-serial-bus.c:335
> 335 info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
>
> What's happening is VIRTIO_CONSOLE_DEVICE_READY is a message for the
> whole device, not for an individual port. So port is NULL. This bug was
> introduced by commit a15bb0d6a981de749452a5180fc8084d625671da.
>
> This commit fixes that by making the port returned by find_port_by_id()
> be used only by the VIRTIO_CONSOLE_PORT_READY and
> VIRTIO_CONSOLE_PORT_OPEN messages.
>
> Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
> ---
> hw/virtio-serial-bus.c | 24 ++++++++++++++----------
> 1 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 9a12104..33a6f61 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -328,18 +328,11 @@ static void handle_control_message(VirtIOSerial *vser,
> void *buf, size_t len)
> cpkt.event = lduw_p(&gcpkt->event);
> cpkt.value = lduw_p(&gcpkt->value);
>
> - port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> - if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
> - return;
> -
> - info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> -
> - switch(cpkt.event) {
> - case VIRTIO_CONSOLE_DEVICE_READY:
> + if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) {
What we lose after this re-arrangement is the check that port is NULL
when this message is received. i.e., a guest bug where port is set to
a valid value when this message arrives. (I think I pointed this out
in a previous mail?)
> if (!cpkt.value) {
> error_report("virtio-serial-bus: Guest failure in adding device
> %s\n",
> vser->bus.qbus.name);
> - break;
> + return;
> }
> /*
> * The device is up, we can now tell the device about all the
> @@ -348,8 +341,19 @@ static void handle_control_message(VirtIOSerial *vser,
> void *buf, size_t len)
> QTAILQ_FOREACH(port, &vser->ports, next) {
> send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1);
> }
> - break;
> + return;
> + }
>
> + port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> + if (!port) {
> + error_report("virtio-serial-bus: Expected port id %d for device
> %s\n",
> + ldl_p(&gcpkt->id), vser->bus.qbus.name);
It's actually 'unexpected port id...' :-)
Also, port id is %u, not %d.
Fixed these two and committed to virtio-serial tree.
Thanks,
Amit
