Re: [PATCH 3/4] net/virtio: avoid passing NULL to qdev_set_parent_bus

[Jens Freimann]

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

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