[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.9] virtio: Fix error handling in virtio_bu
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH for-2.9] virtio: Fix error handling in virtio_bus_device_plugged |
Date: |
Fri, 17 Mar 2017 20:29:25 +0800 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
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
> >
> >