| Message ID | 20191114141613.15804-3-jfreimann@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] net/virtio: fix dev_unplug_pending | expand |
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 <jfreimann@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
On Wed, Nov 20, 2019 at 05:46:36AM -0500, Michael S. Tsirkin wrote: >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 <jfreimann@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. I'll fix this and sent a new version as a single patch. As you said this is not really a series, just individual patches. Thanks! regards Jens
On Wed, Nov 20, 2019 at 01:03:57PM +0100, Jens Freimann wrote: > On Wed, Nov 20, 2019 at 05:46:36AM -0500, Michael S. Tsirkin wrote: > > 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 <jfreimann@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. > > I'll fix this and sent a new version as a single patch. As you said > this is not really a series, just individual patches. > > Thanks! > > regards > Jens I'd just repost them all then. The series was also threaded weirdly (e.g. there's no cover letter instead patches 2 and on link to patch 1).
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; } - 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; }
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 <jfreimann@redhat.com> --- hw/net/virtio-net.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)