Re: [PATCH] virtio: clean up when virtio_queue_set_rings() fails

[Cornelia Huck]

On Wed, 5 Feb 2020 14:49:46 +0000
Stefan Hajnoczi <address@hidden> wrote:

> On Tue, Feb 04, 2020 at 05:02:39PM +0100, Cornelia Huck wrote:
> > On Tue,  4 Feb 2020 15:16:18 +0000
> > Stefan Hajnoczi <address@hidden> wrote:

> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 2c5410e981..5d7f619a1e 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -2163,6 +2163,11 @@ void virtio_queue_set_rings(VirtIODevice *vdev, 
> > > int n, hwaddr desc,
> > >      vdev->vq[n].vring.avail = avail;
> > >      vdev->vq[n].vring.used = used;
> > >      virtio_init_region_cache(vdev, n);
> > > +    if (vdev->broken) {
> > > +        vdev->vq[n].vring.desc = 0;
> > > +        vdev->vq[n].vring.avail = 0;
> > > +        vdev->vq[n].vring.used = 0;
> > > +    }
> > >  }
> > >  
> > >  void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)  
> > 
> > This looks correct; but shouldn't virtio_queue_set_addr() also set
> > .desc to 0 on failure?  
> 
> Now that you mention it, there are a number of other
> virtio_init_region_cache() callers that could be affected.
> 
> I added the error handling code to virtio_queue_set_rings() because
> that's symmetric - this function sets .desc and so it should be the one
> to clear it on error.  But now I think virtio_init_region_cache() should
> take on that responsibility so callers don't need to duplicate this
> error handling code.

Is it clear in every case what the correct error handling procedure
would be? It would feel a bit surprising if the addresses were cleared
for callers that don't directly change them.

pgpwAB_45nUO_.pgp
Description: OpenPGP digital signature