[Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Luiz Capitulino]

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(-)

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 */
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Amit Shah]

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

Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Markus Armbruster]

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.

[...]

Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Amit Shah]

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

Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Luiz Capitulino]

On Fri, 17 Jun 2011 12:17:36 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> 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.

What about moving the VIRTIO_CONSOLE_DEVICE_READY handling out of the
switch, like the patch below? This way the function is divided in a way
that related events are handled together.

I'll implement your first suggestion if you don't like this...

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 579f676..5f96245 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -325,19 +325,12 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         return;
     }
 
-    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;
-
-    switch(cpkt.event) {
-    case VIRTIO_CONSOLE_DEVICE_READY:
+    cpkt.event = lduw_p(&gcpkt->event);
+    if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) {
         if (!cpkt.value) {
-            error_report("virtio-serial-bus: Guest failure in adding device %s\n",
-                         vser->bus.qbus.name);
-            break;
+            error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
+            return;
         }
         /*
          * The device is up, we can now tell the device about all the
@@ -346,8 +339,13 @@ 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));
+    assert(port != NULL);
+
+    switch(cpkt.event) {
     case VIRTIO_CONSOLE_PORT_READY:
         if (!cpkt.value) {
             error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n",

Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Luiz Capitulino]

On Fri, 17 Jun 2011 10:16:44 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Fri, 17 Jun 2011 12:17:36 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > 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.
> 
> What about moving the VIRTIO_CONSOLE_DEVICE_READY handling out of the
> switch, like the patch below? This way the function is divided in a way
> that related events are handled together.
> 
> I'll implement your first suggestion if you don't like this...
> 
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 579f676..5f96245 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -325,19 +325,12 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
>          return;
>      }
>  
> -    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;
> -
> -    switch(cpkt.event) {
> -    case VIRTIO_CONSOLE_DEVICE_READY:
> +    cpkt.event = lduw_p(&gcpkt->event);
> +    if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) {
>          if (!cpkt.value) {
> -            error_report("virtio-serial-bus: Guest failure in adding device %s\n",
> -                         vser->bus.qbus.name);
> -            break;
> +            error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
> +            return;
>          }
>          /*
>           * The device is up, we can now tell the device about all the
> @@ -346,8 +339,13 @@ 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));
> +    assert(port != NULL);

I did this on top of my first patch and forgot to move the DO_UPCAST()
call here... But you got the idea :)

> +
> +    switch(cpkt.event) {
>      case VIRTIO_CONSOLE_PORT_READY:
>          if (!cpkt.value) {
>              error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n",

Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Amit Shah]

On (Fri) 17 Jun 2011 [10:16:44], Luiz Capitulino wrote:
> On Fri, 17 Jun 2011 12:17:36 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > 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.
> 
> What about moving the VIRTIO_CONSOLE_DEVICE_READY handling out of the
> switch, like the patch below? This way the function is divided in a way
> that related events are handled together.
> 
> I'll implement your first suggestion if you don't like this...
> 
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 579f676..5f96245 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -325,19 +325,12 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
>          return;
>      }
>  
> -    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;
> -
> -    switch(cpkt.event) {
> -    case VIRTIO_CONSOLE_DEVICE_READY:
> +    cpkt.event = lduw_p(&gcpkt->event);
> +    if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) {
>          if (!cpkt.value) {
> -            error_report("virtio-serial-bus: Guest failure in adding device %s\n",
> -                         vser->bus.qbus.name);
> -            break;
> +            error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
> +            return;

The line split should remain -- else it goes beyond 80 chars.

>          }
>          /*
>           * The device is up, we can now tell the device about all the
> @@ -346,8 +339,13 @@ 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;
> +    }

Makes me think of one case (totally unrelated to what you found)where
the guest can fool us: by sending multiple VIRTIO_CONSOLE_DEVICE_READY
messages.

> +    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> +    assert(port != NULL);

I doubt if assert is the right thing: if the guest sends bad data, we
shouldn't just kill it.  It's easier to ignore such data, and perhaps
just log it.

> +
> +    switch(cpkt.event) {
>      case VIRTIO_CONSOLE_PORT_READY:
>          if (!cpkt.value) {
>              error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n",

I'm fine with this approach.

		Amit

Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Luiz Capitulino]

On Fri, 17 Jun 2011 21:39:26 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Fri) 17 Jun 2011 [10:16:44], Luiz Capitulino wrote:
> > On Fri, 17 Jun 2011 12:17:36 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> > 
> > > 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.
> > 
> > What about moving the VIRTIO_CONSOLE_DEVICE_READY handling out of the
> > switch, like the patch below? This way the function is divided in a way
> > that related events are handled together.
> > 
> > I'll implement your first suggestion if you don't like this...
> > 
> > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> > index 579f676..5f96245 100644
> > --- a/hw/virtio-serial-bus.c
> > +++ b/hw/virtio-serial-bus.c
> > @@ -325,19 +325,12 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> >          return;
> >      }
> >  
> > -    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;
> > -
> > -    switch(cpkt.event) {
> > -    case VIRTIO_CONSOLE_DEVICE_READY:
> > +    cpkt.event = lduw_p(&gcpkt->event);
> > +    if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) {
> >          if (!cpkt.value) {
> > -            error_report("virtio-serial-bus: Guest failure in adding device %s\n",
> > -                         vser->bus.qbus.name);
> > -            break;
> > +            error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
> > +            return;
> 
> The line split should remain -- else it goes beyond 80 chars.

It's already beyond 80 chars to me.

> 
> >          }
> >          /*
> >           * The device is up, we can now tell the device about all the
> > @@ -346,8 +339,13 @@ 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;
> > +    }
> 
> Makes me think of one case (totally unrelated to what you found)where
> the guest can fool us: by sending multiple VIRTIO_CONSOLE_DEVICE_READY
> messages.

It will be handled just fine, no?

> 
> > +    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> > +    assert(port != NULL);
> 
> I doubt if assert is the right thing: if the guest sends bad data, we
> shouldn't just kill it.  It's easier to ignore such data, and perhaps
> just log it.

Right.

> 
> > +
> > +    switch(cpkt.event) {
> >      case VIRTIO_CONSOLE_PORT_READY:
> >          if (!cpkt.value) {
> >              error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n",
> 
> I'm fine with this approach.
> 
> 		Amit
> 

Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Amit Shah]

On (Fri) 17 Jun 2011 [15:08:11], Luiz Capitulino wrote:

> > >          if (!cpkt.value) {
> > > -            error_report("virtio-serial-bus: Guest failure in adding device %s\n",
> > > -                         vser->bus.qbus.name);
> > > -            break;
> > > +            error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
> > > +            return;
> > 
> > The line split should remain -- else it goes beyond 80 chars.
> 
> It's already beyond 80 chars to me.

I prefer to not break strings that get printed out -- makes it easier
for greppers to find out the source of the string.

> > > @@ -346,8 +339,13 @@ 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;
> > > +    }
> > 
> > Makes me think of one case (totally unrelated to what you found)where
> > the guest can fool us: by sending multiple VIRTIO_CONSOLE_DEVICE_READY
> > messages.
> 
> It will be handled just fine, no?

We'll send out the VIRTIO_CONSOLE_PORT_ADD events for each port
(again).  That's the case now.  No idea how the code might change in
the future and we could end up doing something else in addition which
might be bad.  Anyway, all this is for a buggy or a bad guest.

		Amit

Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Blue Swirl]

On Sat, Jun 18, 2011 at 6:42 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Fri) 17 Jun 2011 [15:08:11], Luiz Capitulino wrote:
>
>> > >          if (!cpkt.value) {
>> > > -            error_report("virtio-serial-bus: Guest failure in adding device %s\n",
>> > > -                         vser->bus.qbus.name);
>> > > -            break;
>> > > +            error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
>> > > +            return;
>> >
>> > The line split should remain -- else it goes beyond 80 chars.
>>
>> It's already beyond 80 chars to me.
>
> I prefer to not break strings that get printed out -- makes it easier
> for greppers to find out the source of the string.

Please read CODING_STYLE and use scripts/checkpatch.pl before
submitting patches.

>> > > @@ -346,8 +339,13 @@ 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;
>> > > +    }
>> >
>> > Makes me think of one case (totally unrelated to what you found)where
>> > the guest can fool us: by sending multiple VIRTIO_CONSOLE_DEVICE_READY
>> > messages.
>>
>> It will be handled just fine, no?
>
> We'll send out the VIRTIO_CONSOLE_PORT_ADD events for each port
> (again).  That's the case now.  No idea how the code might change in
> the future and we could end up doing something else in addition which
> might be bad.  Anyway, all this is for a buggy or a bad guest.
>
>                Amit
>
>

Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Amit Shah]

On (Sun) 19 Jun 2011 [00:43:20], Blue Swirl wrote:
> On Sat, Jun 18, 2011 at 6:42 AM, Amit Shah <amit.shah@redhat.com> wrote:
> > On (Fri) 17 Jun 2011 [15:08:11], Luiz Capitulino wrote:
> >
> >> > >          if (!cpkt.value) {
> >> > > -            error_report("virtio-serial-bus: Guest failure in adding device %s\n",
> >> > > -                         vser->bus.qbus.name);
> >> > > -            break;
> >> > > +            error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
> >> > > +            return;
> >> >
> >> > The line split should remain -- else it goes beyond 80 chars.
> >>
> >> It's already beyond 80 chars to me.
> >
> > I prefer to not break strings that get printed out -- makes it easier
> > for greppers to find out the source of the string.
> 
> Please read CODING_STYLE and use scripts/checkpatch.pl before
> submitting patches.

Good sense should dictate what goes in CODING_STYLE, not the other way
around.

>From dec93d9eccd639f7bfd1343dca65fa112eb19e3e Mon Sep 17 00:00:00 2001
Message-Id: <dec93d9eccd639f7bfd1343dca65fa112eb19e3e.1308718380.git.amit.shah@redhat.com>
From: Amit Shah <amit.shah@redhat.com>
Date: Wed, 22 Jun 2011 10:20:48 +0530
Subject: [PATCH 1/1] CODING_STYLE: Add exception for log output 80-char limit

Output that's logged can be beyond 80 chars to preserve sane grepping.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 CODING_STYLE |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/CODING_STYLE b/CODING_STYLE
index 5ecfa22..886221c 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -31,7 +31,10 @@ Do not leave whitespace dangling off the ends of lines.
 
 2. Line width
 
-Lines are 80 characters; not longer.
+Lines are 80 characters; not longer.  An exception is for output that
+is logged, for example via fprintf() / error_report() functions,
+making it straightforward for people to grep the code for the source
+of the output.
 
 Rationale:
  - Some people like to tile their 24" screens with a 6x4 matrix of 80x24
-- 
1.7.5.4



		Amit

Re: [Qemu-devel] [PATCH] virtio-serial: Fix segfault on guest boot

[Blue Swirl]

On Wed, Jun 22, 2011 at 7:53 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Sun) 19 Jun 2011 [00:43:20], Blue Swirl wrote:
>> On Sat, Jun 18, 2011 at 6:42 AM, Amit Shah <amit.shah@redhat.com> wrote:
>> > On (Fri) 17 Jun 2011 [15:08:11], Luiz Capitulino wrote:
>> >
>> >> > >          if (!cpkt.value) {
>> >> > > -            error_report("virtio-serial-bus: Guest failure in adding device %s\n",
>> >> > > -                         vser->bus.qbus.name);
>> >> > > -            break;
>> >> > > +            error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
>> >> > > +            return;
>> >> >
>> >> > The line split should remain -- else it goes beyond 80 chars.
>> >>
>> >> It's already beyond 80 chars to me.
>> >
>> > I prefer to not break strings that get printed out -- makes it easier
>> > for greppers to find out the source of the string.
>>
>> Please read CODING_STYLE and use scripts/checkpatch.pl before
>> submitting patches.
>
> Good sense should dictate what goes in CODING_STYLE, not the other way
> around.

Unfortunately there is huge amount of bike shedding in this area,
there is no One Good Sense but several which conflict with each other.
Discussions about changing CODING_STYLE haven't been very fruitful.

> From dec93d9eccd639f7bfd1343dca65fa112eb19e3e Mon Sep 17 00:00:00 2001
> Message-Id: <dec93d9eccd639f7bfd1343dca65fa112eb19e3e.1308718380.git.amit.shah@redhat.com>
> From: Amit Shah <amit.shah@redhat.com>
> Date: Wed, 22 Jun 2011 10:20:48 +0530
> Subject: [PATCH 1/1] CODING_STYLE: Add exception for log output 80-char limit
>
> Output that's logged can be beyond 80 chars to preserve sane grepping.

Nack. Grepping (why just logs?) is just one use case. There are other
cases where shorter line length is preferred, like editors, terminals,
patch generation, mail systems munging long lines etc.

>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  CODING_STYLE |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index 5ecfa22..886221c 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -31,7 +31,10 @@ Do not leave whitespace dangling off the ends of lines.
>
>  2. Line width
>
> -Lines are 80 characters; not longer.
> +Lines are 80 characters; not longer.  An exception is for output that
> +is logged, for example via fprintf() / error_report() functions,
> +making it straightforward for people to grep the code for the source
> +of the output.
>
>  Rationale:
>  - Some people like to tile their 24" screens with a 6x4 matrix of 80x24
> --
> 1.7.5.4
>
>
>
>                Amit
>

Re: [Qemu-devel] 80-column rule and breaking output statements

[Amit Shah]

On (Sun) 26 Jun 2011 [20:43:16], Blue Swirl wrote:
> On Wed, Jun 22, 2011 at 7:53 AM, Amit Shah <amit.shah@redhat.com> wrote:

[snip]

> > From dec93d9eccd639f7bfd1343dca65fa112eb19e3e Mon Sep 17 00:00:00 2001
> > Message-Id: <dec93d9eccd639f7bfd1343dca65fa112eb19e3e.1308718380.git.amit.shah@redhat.com>
> > From: Amit Shah <amit.shah@redhat.com>
> > Date: Wed, 22 Jun 2011 10:20:48 +0530
> > Subject: [PATCH 1/1] CODING_STYLE: Add exception for log output 80-char limit
> >
> > Output that's logged can be beyond 80 chars to preserve sane grepping.
> 
> Nack. Grepping (why just logs?) is just one use case. There are other

I'm not talking about grepping logs.  I'm talking about grepping code
once you have something in the logs.  With line breaks in the code but
not in the log, you will not get the desired result.

Example:


fprintf("This is a line ");
fprintf("broken in two code lines\n");


Output is:

This is a line broken in two code lines


Picking that line from the output and grepping for it in the source
doesn't get you what you want.

		Amit

Re: [Qemu-devel] 80-column rule and breaking output statements

[Blue Swirl]

On Mon, Jun 27, 2011 at 12:48 PM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Sun) 26 Jun 2011 [20:43:16], Blue Swirl wrote:
>> On Wed, Jun 22, 2011 at 7:53 AM, Amit Shah <amit.shah@redhat.com> wrote:
>
> [snip]
>
>> > From dec93d9eccd639f7bfd1343dca65fa112eb19e3e Mon Sep 17 00:00:00 2001
>> > Message-Id: <dec93d9eccd639f7bfd1343dca65fa112eb19e3e.1308718380.git.amit.shah@redhat.com>
>> > From: Amit Shah <amit.shah@redhat.com>
>> > Date: Wed, 22 Jun 2011 10:20:48 +0530
>> > Subject: [PATCH 1/1] CODING_STYLE: Add exception for log output 80-char limit
>> >
>> > Output that's logged can be beyond 80 chars to preserve sane grepping.
>>
>> Nack. Grepping (why just logs?) is just one use case. There are other
>
> I'm not talking about grepping logs.  I'm talking about grepping code
> once you have something in the logs.  With line breaks in the code but
> not in the log, you will not get the desired result.
>
> Example:
>
>
> fprintf("This is a line ");
> fprintf("broken in two code lines\n");
>
>
> Output is:
>
> This is a line broken in two code lines
>
>
> Picking that line from the output and grepping for it in the source
> doesn't get you what you want.

Nack. Grep would not match for example
fprintf("'%s' invalid media\n", s);
unless you know how to grep only for some parts of the error string
and in that case splitting the line would not hurt very much anymore.

If you want robust grepping, each error message should contain an ID,
for example:
fprintf("ERROR-ID-0015:'%s' invalid media\n", s);

Then grepping would work even for
fprintf(
"ERROR-ID-0015:"
"'%s'"
" invalid"
" media\n",
s);

Re: [Qemu-devel] 80-column rule and breaking output statements

[Stefan Hajnoczi]

On Fri, Jul 1, 2011 at 8:43 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Jun 27, 2011 at 12:48 PM, Amit Shah <amit.shah@redhat.com> wrote:
>> On (Sun) 26 Jun 2011 [20:43:16], Blue Swirl wrote:
>>> On Wed, Jun 22, 2011 at 7:53 AM, Amit Shah <amit.shah@redhat.com> wrote:
>>
>> [snip]
>>
>>> > From dec93d9eccd639f7bfd1343dca65fa112eb19e3e Mon Sep 17 00:00:00 2001
>>> > Message-Id: <dec93d9eccd639f7bfd1343dca65fa112eb19e3e.1308718380.git.amit.shah@redhat.com>
>>> > From: Amit Shah <amit.shah@redhat.com>
>>> > Date: Wed, 22 Jun 2011 10:20:48 +0530
>>> > Subject: [PATCH 1/1] CODING_STYLE: Add exception for log output 80-char limit
>>> >
>>> > Output that's logged can be beyond 80 chars to preserve sane grepping.
>>>
>>> Nack. Grepping (why just logs?) is just one use case. There are other
>>
>> I'm not talking about grepping logs.  I'm talking about grepping code
>> once you have something in the logs.  With line breaks in the code but
>> not in the log, you will not get the desired result.
>>
>> Example:
>>
>>
>> fprintf("This is a line ");
>> fprintf("broken in two code lines\n");
>>
>>
>> Output is:
>>
>> This is a line broken in two code lines
>>
>>
>> Picking that line from the output and grepping for it in the source
>> doesn't get you what you want.
>
> Nack. Grep would not match for example
> fprintf("'%s' invalid media\n", s);
> unless you know how to grep only for some parts of the error string
> and in that case splitting the line would not hurt very much anymore.
>
> If you want robust grepping, each error message should contain an ID,
> for example:
> fprintf("ERROR-ID-0015:'%s' invalid media\n", s);
>
> Then grepping would work even for
> fprintf(
> "ERROR-ID-0015:"
> "'%s'"
> " invalid"
> " media\n",
> s);

I don't see split lines as an issue because I never grep for an entire
line.  Pick a small, unique, fixed part of the message and you'll find
it.

"Small" in order to avoid any formatting or split line issues.
"Unique" in order to cut down the number of grep results.
"Fixed" in order to avoid format string expansions as Blue Swirl mentioned.

Stefan

Re: [Qemu-devel] 80-column rule and breaking output statements

[Amit Shah]

On (Sat) 02 Jul 2011 [09:38:30], Stefan Hajnoczi wrote:

...

> I don't see split lines as an issue because I never grep for an entire
> line.  Pick a small, unique, fixed part of the message and you'll find
> it.
> 
> "Small" in order to avoid any formatting or split line issues.

Unless you pick your Small string that was split across multiple lines.

> "Unique" in order to cut down the number of grep results.
> "Fixed" in order to avoid format string expansions as Blue Swirl mentioned.

		Amit