On Wed, Nov 20, 2019 at 03:38:58PM +0100, Jens Freimann wrote:
> Make sure we don't pass NULL to qdev_set_parent_bus(). Also simplify
> code a bit and fix a typo.
> This fixes CID 1407224.
>
> Fixes: 9711cd0dfc3f ("net/virtio: add failover support")
> Signed-off-by: Jens Freimann <jfreim...@redhat.com>
patch itself is ok I think
Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
but some questions on the commit log
> ---
> hw/net/virtio-net.c | 42 +++++++++++++++++++++++++-----------------
> 1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index ac4d19109e..78f1e4e87c 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2805,25 +2805,33 @@ static bool failover_replug_primary(VirtIONet *n,
> Error **errp)
> n->primary_device_opts = qemu_opts_from_qdict(
> qemu_find_opts("device"),
> n->primary_device_dict, errp);
> - }
> - if (n->primary_device_opts) {
> - if (n->primary_dev) {
> - n->primary_bus = n->primary_dev->parent_bus;
> - }
> - 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);
> - hotplug_ctrl = qdev_get_hotplug_handler(n->primary_dev);
> - if (hotplug_ctrl) {
> - hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, errp);
> - hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
> + if (!n->primary_device_opts) {
> + error_setg(errp, "virtio_net: couldn't find primary device
> opts");
> + goto out;
> }
> - if (!n->primary_dev) {
> + }
> + if (!n->primary_dev) {
> error_setg(errp, "virtio_net: couldn't find primary device");
> - }
> + goto out;
> }
> - return *errp != NULL;
> +
> + n->primary_bus = n->primary_dev->parent_bus;
> + if (!n->primary_bus) {
> + error_setg(errp, "virtio_net: couldn't find primary bus");
> + goto out;
> + }
> + 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);
> + hotplug_ctrl = qdev_get_hotplug_handler(n->primary_dev);
> + if (hotplug_ctrl) {
> + hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, errp);
> + hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
> + }
> +
> +out:
> + return *errp == NULL;
> }
>
> static void virtio_net_handle_migration_primary(VirtIONet *n,
So the logic actually was inverted here? Then ... how did this work?
and how was it tested?
Also, pls mention the change in the commit log.
> @@ -2852,7 +2860,7 @@ static void
> virtio_net_handle_migration_primary(VirtIONet *n,
> warn_report("couldn't unplug primary device");
> }
> } else if (migration_has_failed(s)) {
> - /* We already unplugged the device let's plugged it back */
> + /* We already unplugged the device let's plug it back */
> if (!failover_replug_primary(n, &err)) {
> if (err) {
> error_report_err(err);
> --
> 2.21.0
