Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: send_control_msg should not deal with cpkts

[Amit Shah <amit.shah@redhat.com>]

On (Mon) 17 Dec 2012 [14:14:17], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > Stuff the cpkt before calling send_control_msg().  it should not be
> > concerned about contents, just send across the buffer.
> >
> > A few code refactorings recently have made mkaing this change easier
> > than earlier.

Ugh, I'll fix the typo and incoherent language here before merging.

> > Coverity and clang have flagged this code several times in the past
> > (cpkt->id not set before send_control_event() passed it on to
> > send_control_msg()).  This will finally eliminate the false-positive.
> >
> > CC: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> Patch makes sense to me, and the Coverity defect report is gone.

Thanks for checking!

> However, it now worries find_port_by_id() in remove_port() could return
> a null pointer, which is then dereferenced.  No idea why it didn't
> report that before.  Obvious suppressor:
> 
>     diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>     index 47d0481..7ff7505 100644
>     --- a/hw/virtio-serial-bus.c
>     +++ b/hw/virtio-serial-bus.c
>     @@ -826,6 +826,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
>          vser->ports_map[i] &= ~(1U << (port_id % 32));
> 
>          port = find_port_by_id(vser, port_id);
>     +    assert(port);
>          /* Flush out any unconsumed buffers first */
>          discard_vq_data(port->ovq, &port->vser->vdev);

remove_port() is called by the hot-unplug qdev callback, and if the
port's missing from our tailq, something's gone wrong anyway.  So this
patch makes sense too.

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

		Amit