On Thu, Nov 14, 2019 at 03:16:12PM +0100, Jens Freimann wrote: > Make sure no arguments for qdev_set_parent_bus are NULL. > This fixes CID 1407224. > > Fixes: 9711cd0dfc3f ("net/virtio: add failover support") > Signed-off-by: Jens Freimann <jfreim...@redhat.com> > --- > hw/net/virtio-net.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index ac4d19109e..81650d4dc0 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -2809,8 +2809,16 @@ static bool failover_replug_primary(VirtIONet *n, > Error **errp) > if (n->primary_device_opts) { > if (n->primary_dev) { > n->primary_bus = n->primary_dev->parent_bus; > + if (n->primary_bus) { > + qdev_set_parent_bus(n->primary_dev, n->primary_bus); > + } else { > + error_setg(errp, "virtio_net: couldn't set bus for primary"); > + goto out; > + } > + } else { > + error_setg(errp, "virtio_net: couldn't find primary device"); > + goto out; > }
A bit less messy: if (!n->primary_dev) { error_setg(errp, "virtio_net: couldn't find primary device"); goto err; } n->primary_bus = n->primary_dev->parent_bus; if (!n->primary_bus) { error_setg(errp, "virtio_net: couldn't find primary device"); goto err; } qdev_set_parent_bus(n->primary_dev, n->primary_bus); err: return false; also is it valid for primary_device_opts to not be present at all? Maybe we should check that too. > - qdev_set_parent_bus(n->primary_dev, n->primary_bus); > n->primary_should_be_hidden = false; > qemu_opt_set_bool(n->primary_device_opts, > "partially_hotplugged", true, errp); > @@ -2819,10 +2827,8 @@ static bool failover_replug_primary(VirtIONet *n, > Error **errp) > hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, errp); > hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp); > } > - if (!n->primary_dev) { > - error_setg(errp, "virtio_net: couldn't find primary device"); > - } > } > +out: > return *errp != NULL; > } I'd handle errors separately from good path. BTW I don't understand something here: *errp != NULL is true on error, no? Why are we returning success? Confused. Just return true would be cleaner imho. > > -- > 2.21.0