| Message ID | 147446364924.4880.15558748866518264841.stgit@bahia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 21 Sep 2016 15:14:09 +0200 Greg Kurz <groug@kaod.org> wrote: > All these errors are caused by a buggy guest: let's switch the device to > the broken state instead of terminating QEMU. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/block/virtio-blk.c | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 3a6112fbf4c4..1285d196a40f 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -468,30 +468,32 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev, > return true; > } > > -void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > +int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) Unrelated to your patch: It seems there are no callers (left) outside of this file; should the function be made static? Related to your patch: You didn't change the prototype in the header :) (...) > @@ -586,7 +589,9 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) > blk_io_plug(s->blk); > > while ((req = virtio_blk_get_request(s, vq))) { > - virtio_blk_handle_request(req, &mrb); > + if (virtio_blk_handle_request(req, &mrb)) { > + return; Does the missing blk_io_unplug() have any side-effects outside of this device, which is broken anyway? > + } > } > > if (mrb.num_reqs) {
On Wed, 21 Sep 2016 16:28:34 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Wed, 21 Sep 2016 15:14:09 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > All these errors are caused by a buggy guest: let's switch the device to > > the broken state instead of terminating QEMU. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/block/virtio-blk.c | 27 +++++++++++++++++---------- > > 1 file changed, 17 insertions(+), 10 deletions(-) > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index 3a6112fbf4c4..1285d196a40f 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -468,30 +468,32 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev, > > return true; > > } > > > > -void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > > +int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > > Unrelated to your patch: It seems there are no callers (left) outside > of this file; should the function be made static? > Yeah you're right. The dataplane does not call this function anymore since: "03de2f527499 virtio-blk: do not use vring in dataplane" I'll fix this in a preparatory patch. > Related to your patch: You didn't change the prototype in the header :) > Oops... caught in the act of not running a final build before sending :) > (...) > > > @@ -586,7 +589,9 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) > > blk_io_plug(s->blk); > > > > while ((req = virtio_blk_get_request(s, vq))) { > > - virtio_blk_handle_request(req, &mrb); > > + if (virtio_blk_handle_request(req, &mrb)) { > > + return; > > Does the missing blk_io_unplug() have any side-effects outside of this > device, which is broken anyway? > Ugh, this is not intentional... and since it may go down many paths, I won't bet there are no side effects. I'll fix this with an explicit err_out: label. > > + } > > } > > > > if (mrb.num_reqs) { > >
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 3a6112fbf4c4..1285d196a40f 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -468,30 +468,32 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev, return true; } -void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) +int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) { uint32_t type; struct iovec *in_iov = req->elem.in_sg; struct iovec *iov = req->elem.out_sg; unsigned in_num = req->elem.in_num; unsigned out_num = req->elem.out_num; + VirtIOBlock *s = req->dev; + VirtIODevice *vdev = VIRTIO_DEVICE(s); if (req->elem.out_num < 1 || req->elem.in_num < 1) { - error_report("virtio-blk missing headers"); - exit(1); + virtio_error(vdev, "virtio-blk missing headers"); + return -1; } if (unlikely(iov_to_buf(iov, out_num, 0, &req->out, sizeof(req->out)) != sizeof(req->out))) { - error_report("virtio-blk request outhdr too short"); - exit(1); + virtio_error(vdev, "virtio-blk request outhdr too short"); + return -1; } iov_discard_front(&iov, &out_num, sizeof(req->out)); if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) { - error_report("virtio-blk request inhdr too short"); - exit(1); + virtio_error(vdev, "virtio-blk request inhdr too short"); + return -1; } /* We always touch the last byte, so just see how big in_iov is. */ @@ -529,7 +531,7 @@ void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) block_acct_invalid(blk_get_stats(req->dev->blk), is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ); virtio_blk_free_request(req); - return; + return 0; } block_acct_start(blk_get_stats(req->dev->blk), @@ -576,6 +578,7 @@ void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP); virtio_blk_free_request(req); } + return 0; } void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) @@ -586,7 +589,9 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq) blk_io_plug(s->blk); while ((req = virtio_blk_get_request(s, vq))) { - virtio_blk_handle_request(req, &mrb); + if (virtio_blk_handle_request(req, &mrb)) { + return; + } } if (mrb.num_reqs) { @@ -625,7 +630,9 @@ static void virtio_blk_dma_restart_bh(void *opaque) while (req) { VirtIOBlockReq *next = req->next; - virtio_blk_handle_request(req, &mrb); + if (virtio_blk_handle_request(req, &mrb)) { + return; + } req = next; }
All these errors are caused by a buggy guest: let's switch the device to the broken state instead of terminating QEMU. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/block/virtio-blk.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)