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 <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.
>
> 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).
--
MST
