[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/4] net/virtio: avoid passing NULL to qdev_set_parent_bus
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH 3/4] net/virtio: avoid passing NULL to qdev_set_parent_bus |
Date: |
Wed, 20 Nov 2019 05:46:36 -0500 |
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 <address@hidden>
> ---
> 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