[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/18] virtio: Return error from virtqueue_next_
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 04/18] virtio: Return error from virtqueue_next_desc |
Date: |
Tue, 21 Apr 2015 11:56:32 +0200 |
On Tue, Apr 21, 2015 at 03:30:23PM +0800, Fam Zheng wrote:
> On Tue, 04/21 08:37, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2015 at 03:59:19PM +0800, Fam Zheng wrote:
> > > Two callers pass error_abort now, which can be changed to check return
> > > value
> > > and pass the error on.
> > >
> > > Signed-off-by: Fam Zheng <address@hidden>
> > > ---
> > > hw/virtio/virtio.c | 27 ++++++++++++++++++---------
> > > 1 file changed, 18 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index a525f8e..2a24829 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -329,10 +329,11 @@ static int virtqueue_get_head(VirtQueue *vq,
> > > unsigned int idx,
> > > return head;
> > > }
> > >
> > > -static unsigned virtqueue_next_desc(VirtIODevice *vdev, hwaddr desc_pa,
> > > - unsigned int i, unsigned int max)
> > > +static int virtqueue_next_desc(VirtIODevice *vdev, hwaddr desc_pa,
> > > + unsigned int i, unsigned int max,
> > > + Error **errp)
> > > {
> > > - unsigned int next;
> > > + int next;
> > >
> > > /* If this descriptor says it doesn't chain, we're done. */
> > > if (!(vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_NEXT)) {
> > > @@ -345,8 +346,8 @@ static unsigned virtqueue_next_desc(VirtIODevice
> > > *vdev, hwaddr desc_pa,
> > > smp_wmb();
> > >
> > > if (next >= max) {
> > > - error_report("Desc next is %u", next);
> > > - exit(1);
> > > + error_setg(errp, "Desc next is %u", next);
> > > + return -EINVAL;
> >
> > I think it's best to return max here. No need to change return type
> > then.
>
> We use negative return code elsewherer for reporting errors, I personally
> prefer -EINVAL. Are you concerned about overflow?
Yes.
> >
> > > }
> > >
> > > return next;
> > > @@ -392,7 +393,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq,
> > > unsigned int *in_bytes,
> > > num_bufs = i = 0;
> > > }
> > >
> > > - do {
> > > + while (true) {
> > > /* If we've got too many, that implies a descriptor loop. */
> > > if (++num_bufs > max) {
> > > error_report("Looped descriptor");
> > > @@ -407,7 +408,11 @@ void virtqueue_get_avail_bytes(VirtQueue *vq,
> > > unsigned int *in_bytes,
> > > if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
> > > goto done;
> > > }
> > > - } while ((i = virtqueue_next_desc(vdev, desc_pa, i, max)) !=
> > > max);
> > > + i = virtqueue_next_desc(vdev, desc_pa, i, max, &error_abort);
> > > + if (i == max) {
> > > + break;
> > > + }
> > > + }
> > >
> > > if (!indirect)
> > > total_bufs = num_bufs;
> > > @@ -493,7 +498,7 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement
> > > *elem)
> > > }
> > >
> > > /* Collect all the descriptors */
> > > - do {
> > > + while (true) {
> > > struct iovec *sg;
> > >
> > > if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_WRITE) {
> > > @@ -519,7 +524,11 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement
> > > *elem)
> > > error_report("Looped descriptor");
> > > exit(1);
> > > }
> > > - } while ((i = virtqueue_next_desc(vdev, desc_pa, i, max)) != max);
> > > + i = virtqueue_next_desc(vdev, desc_pa, i, max, &error_abort);
> > > + if (i == max) {
> > > + break;
> > > + }
> > > + }
> > >
> >
> > Why refactor this as part of this patch?
>
> Graceful error handling will need to un-inline the loop condition, so refactor
> it as we're touching the line.
>
> Fam
I don't think adding a ton of untested paths is a good strategy for
error-handling. When you detect an error, report it then go back to
normal path as quickly as possible. In this case, reporting
ring empty to caller will make caller stop which is
exactly what we want.
> >
> > > /* Now map what we have collected */
> > > virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1,
> > > --
> > > 1.9.3
- [Qemu-devel] [PATCH 12/18] virtio: Return -EINVAL if the vdev needs reset in virtqueue_pop, (continued)
- [Qemu-devel] [PATCH 12/18] virtio: Return -EINVAL if the vdev needs reset in virtqueue_pop, Fam Zheng, 2015/04/17
- [Qemu-devel] [PATCH 13/18] virtio-blk: Graceful error handling of virtqueue_pop, Fam Zheng, 2015/04/17
- [Qemu-devel] [PATCH 14/18] qtest: Add "QTEST_FILTER" to filter test cases, Fam Zheng, 2015/04/17
- [Qemu-devel] [PATCH 15/18] qtest: virtio-blk: Extract "setup" for future reuse, Fam Zheng, 2015/04/17
- [Qemu-devel] [PATCH 16/18] libqos: Add qvirtio_needs_reset, Fam Zheng, 2015/04/17
- [Qemu-devel] [PATCH 17/18] qtest: Add test case for "needs reset" of virtio-blk, Fam Zheng, 2015/04/17
- [Qemu-devel] [PATCH 18/18] qtest: virtio-blk: Suppress virtio error messages in "make check", Fam Zheng, 2015/04/17
- [Qemu-devel] [PATCH 04/18] virtio: Return error from virtqueue_next_desc, Fam Zheng, 2015/04/17
- [Qemu-devel] [PATCH 08/18] virtio: Return error from virtio_add_queue, Fam Zheng, 2015/04/17
- Re: [Qemu-devel] [PATCH 00/18] virtio-blk: Support "VIRTIO_CONFIG_S_NEEDS_RESET", Cornelia Huck, 2015/04/20
- Re: [Qemu-devel] [PATCH 00/18] virtio-blk: Support "VIRTIO_CONFIG_S_NEEDS_RESET", Fam Zheng, 2015/04/21
- Re: [Qemu-devel] [PATCH 00/18] virtio-blk: Support "VIRTIO_CONFIG_S_NEEDS_RESET", Cornelia Huck, 2015/04/21
- Re: [Qemu-devel] [PATCH 00/18] virtio-blk: Support "VIRTIO_CONFIG_S_NEEDS_RESET", Fam Zheng, 2015/04/21
- Re: [Qemu-devel] [PATCH 00/18] virtio-blk: Support "VIRTIO_CONFIG_S_NEEDS_RESET", Cornelia Huck, 2015/04/21
- Re: [Qemu-devel] [PATCH 00/18] virtio-blk: Support "VIRTIO_CONFIG_S_NEEDS_RESET", Fam Zheng, 2015/04/21
- Re: [Qemu-devel] [PATCH 00/18] virtio-blk: Support "VIRTIO_CONFIG_S_NEEDS_RESET", Cornelia Huck, 2015/04/21
- Re: [Qemu-devel] [PATCH 00/18] virtio-blk: Support "VIRTIO_CONFIG_S_NEEDS_RESET", Michael S. Tsirkin, 2015/04/21
Re: [Qemu-devel] [PATCH 00/18] virtio-blk: Support "VIRTIO_CONFIG_S_NEEDS_RESET", Michael S. Tsirkin, 2015/04/20