Re: [Qemu-devel] [PATCH for-2.9] virtio: Fix error handling in virtio_bus_device_plugged

[Fam Zheng]

On Fri, 03/17 13:18, Andrew Jones wrote:
> On Fri, Mar 17, 2017 at 07:33:53PM +0800, Fam Zheng wrote:
> > For one thing we shouldn't continue if an error happened, for the other
> > two steps failing can cause an abort() in error_setg because we reuse
> > the same errp blindly.
> > 
> > Add error handling checks to fix both issues.
> > 
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  hw/virtio/virtio-bus.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> > index a886011..ef76919 100644
> > --- a/hw/virtio/virtio-bus.c
> > +++ b/hw/virtio/virtio-bus.c
> > @@ -25,6 +25,7 @@
> >  #include "qemu/osdep.h"
> >  #include "hw/hw.h"
> >  #include "qemu/error-report.h"
> > +#include "qapi/error.h"
> >  #include "hw/qdev.h"
> >  #include "hw/virtio/virtio-bus.h"
> >  #include "hw/virtio/virtio.h"
> > @@ -48,21 +49,34 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, 
> > Error **errp)
> >      VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
> >      VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> >      bool has_iommu = virtio_host_has_feature(vdev, 
> > VIRTIO_F_IOMMU_PLATFORM);
> > +    Error *local_err = NULL;
> >  
> >      DPRINTF("%s: plug device.\n", qbus->name);
> >  
> >      if (klass->pre_plugged != NULL) {
> > -        klass->pre_plugged(qbus->parent, errp);
> > +        klass->pre_plugged(qbus->parent, &local_err);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            return;
> > +        }
> >      }
> >  
> >      /* Get the features of the plugged device. */
> >      assert(vdc->get_features != NULL);
> >      vdev->host_features = vdc->get_features(vdev, vdev->host_features,
> > -                                            errp);
> > +                                            &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> >  
> >      if (klass->device_plugged != NULL) {
> >          klass->device_plugged(qbus->parent, errp);
> 
> Did you intend to change this errp to &local_err, as well? If not,
> then the body of the below hunk is unreachable, as local_err can
> never be non-null there.

Yes, this is an overlook.

Fam

> 
> Thanks,
> drew
> 
> >      }
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> >  
> >      if (klass->get_dma_as != NULL && has_iommu) {
> >          virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> > -- 
> > 2.9.3
> > 
> >