| Message ID | 1322497100-24167-1-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 11/29/2011 12:18 AM, Stefan Hajnoczi wrote: > Callers of bdrv_is_allocated() may go beyond the end of the image. For > general robustness we should limit to the end of the image so that > callers don't end up using out-of-range sector counts and receive -EIO. It seems the same problem exists in qcow/qcow2, why do we not add the limit in bdrv_is_allocated? Then it can cover all formats. Actually, I can't figure out how the caller can go beyond the end of the image. If the I/O request for sectors beyond image size comes from guest, it should be dropped in the guest block layer. If the request comes from qemu, like block streaming, it should also honor the image size, right? Mark.
On Tue, Nov 29, 2011 at 5:01 AM, Mark Wu <wudxw@linux.vnet.ibm.com> wrote: > On 11/29/2011 12:18 AM, Stefan Hajnoczi wrote: >> >> Callers of bdrv_is_allocated() may go beyond the end of the image. For >> general robustness we should limit to the end of the image so that >> callers don't end up using out-of-range sector counts and receive -EIO. > > It seems the same problem exists in qcow/qcow2, why do we not add the limit > in bdrv_is_allocated? Then it can cover all formats. > > Actually, I can't figure out how the caller can go beyond the end of the > image. If the I/O request for sectors beyond image size comes from guest, > it should be dropped in the guest block layer. If the request comes from > qemu, like block streaming, it should also honor the image size, right? Today's bdrv_is_allocated() implementation limits to the end of device if BlockDriver.bdrv_is_allocated is NULL (there is a default implementation). This is a hint that callers may pass pnum values beyond the end of the image. Why is it useful to allow this? Because it makes the caller's job simpler. They can pass in their buffer size directly instead of checking for end of device. The result from bdrv_is_allocated() should limit to the end of device and the caller can safely use it. I think we should preserve this behavior. I like your idea of making it generic and will do that in a new patch. Stefan
diff --git a/block/qed-cluster.c b/block/qed-cluster.c index f64b2af..4632b32 100644 --- a/block/qed-cluster.c +++ b/block/qed-cluster.c @@ -142,6 +142,9 @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, */ len = MIN(len, (((pos >> s->l1_shift) + 1) << s->l1_shift) - pos); + /* Limit length to image size */ + len = MIN(len, s->header.image_size - pos); + l2_offset = s->l1_table->offsets[qed_l1_index(s, pos)]; if (qed_offset_is_unalloc_cluster(l2_offset)) { cb(opaque, QED_CLUSTER_L1, 0, len);
Callers of bdrv_is_allocated() may go beyond the end of the image. For general robustness we should limit to the end of the image so that callers don't end up using out-of-range sector counts and receive -EIO. This fix will allow the image streaming to terminate successfully. Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- block/qed-cluster.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)