| Message ID | 20110616133849.02fb4bea@doriath |
|---|---|
| State | New |
| Headers | show |
On (Thu) 16 Jun 2011 [13:38:49], 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); Strange, I've not seen it so far in my testing (neither in the daily test runs of the virtio-serial testsuite). > I've also bisected this and git points out to commit: > > commit a15bb0d6a981de749452a5180fc8084d625671da > Author: Markus Armbruster <armbru@redhat.com> > Date: Wed May 25 14:21:13 2011 +0200 > > virtio-serial: Drop redundant VirtIOSerialPort member info > > I think what's happening is that the device is not initialized on a > VIRTIO_CONSOLE_DEVICE_READY event. Moving the DO_UPCAST() call to > the other events fixes the problem to me. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > hw/virtio-serial-bus.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c > index 9a12104..579f676 100644 > --- a/hw/virtio-serial-bus.c > +++ b/hw/virtio-serial-bus.c > @@ -332,8 +332,6 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) > if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY) > return; > > - info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info); > - Ah - this missed the !port check. It should be possible to do this in a 'if (port)' block instead of replicating in the individual case statements. Thanks for the debugging and patch; please update with the above and I'll apply it to the virtio-serial tree. Amit
Amit Shah <amit.shah@redhat.com> writes: > On (Thu) 16 Jun 2011 [13:38:49], 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); > > Strange, I've not seen it so far in my testing (neither in the daily > test runs of the virtio-serial testsuite). > >> I've also bisected this and git points out to commit: >> >> commit a15bb0d6a981de749452a5180fc8084d625671da >> Author: Markus Armbruster <armbru@redhat.com> >> Date: Wed May 25 14:21:13 2011 +0200 >> >> virtio-serial: Drop redundant VirtIOSerialPort member info >> >> I think what's happening is that the device is not initialized on a >> VIRTIO_CONSOLE_DEVICE_READY event. Really? I believe the device is initialized just fine, but the message refers to a nonexistant port. The check after find_port_by_id() suggests that's expected for VIRTIO_CONSOLE_DEVICE_READY. >> Moving the DO_UPCAST() call to >> the other events fixes the problem to me. >> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >> --- >> hw/virtio-serial-bus.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c >> index 9a12104..579f676 100644 >> --- a/hw/virtio-serial-bus.c >> +++ b/hw/virtio-serial-bus.c >> @@ -332,8 +332,6 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) >> if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY) >> return; >> >> - info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info); >> - > > Ah - this missed the !port check. It should be possible to do this in > a 'if (port)' block instead of replicating in the individual case > statements. You might have to do something like info = port ? DO_UPCAST(...) : NULL to avoid warnings about info used uninitialized. [...]
On (Fri) 17 Jun 2011 [09:47:31], Markus Armbruster wrote: > Amit Shah <amit.shah@redhat.com> writes: > > > On (Thu) 16 Jun 2011 [13:38:49], 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); > > > > Strange, I've not seen it so far in my testing (neither in the daily > > test runs of the virtio-serial testsuite). > > > >> I've also bisected this and git points out to commit: > >> > >> commit a15bb0d6a981de749452a5180fc8084d625671da > >> Author: Markus Armbruster <armbru@redhat.com> > >> Date: Wed May 25 14:21:13 2011 +0200 > >> > >> virtio-serial: Drop redundant VirtIOSerialPort member info > >> > >> I think what's happening is that the device is not initialized on a > >> VIRTIO_CONSOLE_DEVICE_READY event. > > Really? > > I believe the device is initialized just fine, but the message refers to > a nonexistant port. The check after find_port_by_id() suggests that's > expected for VIRTIO_CONSOLE_DEVICE_READY. What's happening is VIRTIO_CONSOLE_DEVICE_READY is a message for the whole device, not for an individual port. So port is NULL. All other messages (currently) are port-specific, so they have 'port' set to something. The commit message needs to be tweaked for this. > > >> Moving the DO_UPCAST() call to > >> the other events fixes the problem to me. > >> > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > >> --- > >> hw/virtio-serial-bus.c | 4 ++-- > >> 1 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c > >> index 9a12104..579f676 100644 > >> --- a/hw/virtio-serial-bus.c > >> +++ b/hw/virtio-serial-bus.c > >> @@ -332,8 +332,6 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) > >> if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY) > >> return; > >> > >> - info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info); > >> - > > > > Ah - this missed the !port check. It should be possible to do this in > > a 'if (port)' block instead of replicating in the individual case > > statements. > > You might have to do something like info = port ? DO_UPCAST(...) : NULL > to avoid warnings about info used uninitialized. That won't happen -- in the case where port is NULL, we shouldn't try to use info. If we do get such a warning, it means there's a bug :-) Amit
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 9a12104..579f676 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -332,8 +332,6 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) 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.value) { @@ -363,6 +361,7 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) * this port is a console port so that the guest can hook it * up to hvc. */ + info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info); if (info->is_console) { send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1); } @@ -398,6 +397,7 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len) break; case VIRTIO_CONSOLE_PORT_OPEN: + info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info); port->guest_connected = cpkt.value; if (cpkt.value && info->guest_open) { /* Send the guest opened notification if an app is interested */
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); I've also bisected this and git points out to commit: commit a15bb0d6a981de749452a5180fc8084d625671da Author: Markus Armbruster <armbru@redhat.com> Date: Wed May 25 14:21:13 2011 +0200 virtio-serial: Drop redundant VirtIOSerialPort member info I think what's happening is that the device is not initialized on a VIRTIO_CONSOLE_DEVICE_READY event. Moving the DO_UPCAST() call to the other events fixes the problem to me. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- hw/virtio-serial-bus.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)