| Message ID | 1391784474-418-1-git-send-email-olaf@aepfle.de |
|---|---|
| State | New |
| Headers | show |
On Fri, 7 Feb 2014, Olaf Hering wrote: > bdrv_acct_done was called unconditional. But in case the ioreq has no > segments there is no matching bdrv_acct_start call. This could lead to > bogus accounting values. > > Found by code inspection. > > Signed-off-by: Olaf Hering <olaf@aepfle.de> added to my queue, I'll fix the tab manually > v2: > add comment /* fall through */ > > hw/block/xen_disk.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c > index 098f6c6..fb4ca4a 100644 > --- a/hw/block/xen_disk.c > +++ b/hw/block/xen_disk.c > @@ -483,7 +483,19 @@ static void qemu_aio_complete(void *opaque, int ret) > ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY; > ioreq_unmap(ioreq); > ioreq_finish(ioreq); > - bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct); > + switch (ioreq->req.operation) { > + case BLKIF_OP_WRITE: > + case BLKIF_OP_FLUSH_DISKCACHE: > + if (!ioreq->req.nr_segments) { > + break; > + } > + /* fall through */ > + case BLKIF_OP_READ: > + bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct); > + break; > + default: > + break; > + } > qemu_bh_schedule(ioreq->blkdev->bh); > } > >
On Fri, Feb 07, Stefano Stabellini wrote:
> added to my queue, I'll fix the tab manually
Thanks, sorry for the TAB.
Olaf
Am 07.02.2014 um 15:47 hat Olaf Hering geschrieben: > bdrv_acct_done was called unconditional. But in case the ioreq has no > segments there is no matching bdrv_acct_start call. This could lead to > bogus accounting values. > > Found by code inspection. > > Signed-off-by: Olaf Hering <olaf@aepfle.de> > --- > v2: > add comment /* fall through */ > > hw/block/xen_disk.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c > index 098f6c6..fb4ca4a 100644 > --- a/hw/block/xen_disk.c > +++ b/hw/block/xen_disk.c > @@ -483,7 +483,19 @@ static void qemu_aio_complete(void *opaque, int ret) > ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY; > ioreq_unmap(ioreq); > ioreq_finish(ioreq); > - bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct); > + switch (ioreq->req.operation) { > + case BLKIF_OP_WRITE: > + case BLKIF_OP_FLUSH_DISKCACHE: > + if (!ioreq->req.nr_segments) { > + break; > + } > + /* fall through */ > + case BLKIF_OP_READ: > + bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct); > + break; > + default: > + break; > + } > qemu_bh_schedule(ioreq->blkdev->bh); > } I think the default case could actually be abort() because it should never happen. But either way, you can add: Acked-by: Kevin Wolf <kwolf@redhat.com>
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c index 098f6c6..fb4ca4a 100644 --- a/hw/block/xen_disk.c +++ b/hw/block/xen_disk.c @@ -483,7 +483,19 @@ static void qemu_aio_complete(void *opaque, int ret) ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY; ioreq_unmap(ioreq); ioreq_finish(ioreq); - bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct); + switch (ioreq->req.operation) { + case BLKIF_OP_WRITE: + case BLKIF_OP_FLUSH_DISKCACHE: + if (!ioreq->req.nr_segments) { + break; + } + /* fall through */ + case BLKIF_OP_READ: + bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct); + break; + default: + break; + } qemu_bh_schedule(ioreq->blkdev->bh); }
bdrv_acct_done was called unconditional. But in case the ioreq has no segments there is no matching bdrv_acct_start call. This could lead to bogus accounting values. Found by code inspection. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- v2: add comment /* fall through */ hw/block/xen_disk.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)