| Message ID | 20191120143859.24584-4-jfreimann@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | net/virtio: fixes for failover | expand |
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 <jfreimann@redhat.com> patch itself is ok I think Reviewed-by: Michael S. Tsirkin <mst@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
On Wed, Nov 20, 2019 at 10:04:01AM -0500, Michael S. Tsirkin wrote: >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 <jfreimann@redhat.com> > >patch itself is ok I think > >Reviewed-by: Michael S. Tsirkin <mst@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? It did not work and I missed the error in my test output. I tested it now manually by migrating and then cancelling. Device was added back to source VM successfully. Will sent new version with better patch description. Thanks! regards Jens
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, @@ -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);
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 <jfreimann@redhat.com> --- hw/net/virtio-net.c | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-)