On 2015-02-23 at 09:27, Peter Lieven wrote:
Signed-off-by: Peter Lieven <p...@kamp.de>
---
block/vpc.c | 116 +++++++++++++++++++++++++++--------------------------------
1 file changed, 52 insertions(+), 64 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index 326c2bb..4e5ba85 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -497,40 +497,70 @@ static int vpc_get_info(BlockDriverState *bs,
BlockDriverInfo *bdi)
return 0;
}
-static int vpc_read(BlockDriverState *bs, int64_t sector_num,
- uint8_t *buf, int nb_sectors)
+static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
+ int64_t sector_num, int nb_sectors, int *pnum)
How about just putting the function header here? If you really have to move
vpc_co_get_block_status() up, I'd rather like it to be in a separate patch.
Second, while apparently vpc_read() is actually called in a coroutine, that is pretty hard to know. Most importantly, it's not marked as a coroutine_fn. Therefore I don't think it's a good idea to call vpc_co_get_block_status() directly; I'd vote for
either using bdrv_get_block_status(), or moving the content of vpc_co_get_block_status() to a non-coroutine_fn (it doesn't contain an coroutine-related function calls, so this is fine) and then making vpc_co_get_block_status() a wrapper around that, or
just dropping this patch.
The latter I'm proposing because I don't really see what this patch improves.
The previous vpc_read() function was pretty straightforward, too, and I don't
think it was unbearably longer.
One could argue that the coroutine_fn stuff doesn't really matter in this situation because it doesn't actually do anything right now and vpc_co_get_block_status() does not call any other coroutine_fn functions in turn; however, it is a semantic
contract established by include/block/coroutine.h and as far as I remember, Stefan did eventually want to have something to error out on compile-time if a non-coroutine_fn function calls a coroutine_fn. I don't like breaking this contract even if it's
not bad in this specific case.
Considering you probably think bdrv_get_block_status() to be too much overhead (it will fall down to the protocol layer on VHD_FIXED) and you probably find making vpc_read() shorter justified (again, which I don't necessarily), I think moving the
contents of vpc_co_get_block_status() to a non-coroutine_fn might be the best way to go.
{
BDRVVPCState *s = bs->opaque;
- int ret;
- int64_t offset;
- int64_t sectors, sectors_per_block;
- VHDFooter *footer = (VHDFooter *) s->footer_buf;
+ VHDFooter *footer = (VHDFooter*) s->footer_buf;
+ int64_t start, offset;
+ bool allocated;
+ int n;
if (be32_to_cpu(footer->type) == VHD_FIXED) {
- return bdrv_read(bs->file, sector_num, buf, nb_sectors);
+ *pnum = nb_sectors;
+ return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+ (sector_num << BDRV_SECTOR_BITS);
}
- while (nb_sectors > 0) {
- offset = get_sector_offset(bs, sector_num, 0);
- sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
- sectors = sectors_per_block - (sector_num % sectors_per_block);
- if (sectors > nb_sectors) {
- sectors = nb_sectors;
+ offset = get_sector_offset(bs, sector_num, 0);
+ start = offset;
+ allocated = (offset != -1);
+ *pnum = 0;
+
+ do {
+ /* All sectors in a block are contiguous (without using the bitmap) */
+ n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
+ - sector_num;
+ n = MIN(n, nb_sectors);
+
+ *pnum += n;
+ sector_num += n;
+ nb_sectors -= n;
+
+ if (allocated) {
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
}
+ if (nb_sectors == 0) {
+ break;
+ }
+ offset = get_sector_offset(bs, sector_num, 0);
+ } while (offset == -1);
- if (offset == -1) {
- memset(buf, 0, sectors * BDRV_SECTOR_SIZE);
- } else {
- ret = bdrv_pread(bs->file, offset, buf,
- sectors * BDRV_SECTOR_SIZE);
- if (ret != sectors * BDRV_SECTOR_SIZE) {
+ return 0;
+}
+
+static int vpc_read(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors)
+{
+ int ret, n;
+ int64_t ret2;
+
+ while (nb_sectors > 0) {
+ ret2 = vpc_co_get_block_status(bs, sector_num, nb_sectors, &n);
+
Superfluous whitespace here.
+ if (ret2 & BDRV_BLOCK_OFFSET_VALID) {
+ ret = bdrv_pread(bs->file, ret2 & BDRV_SECTOR_MASK, buf,
+ n * BDRV_SECTOR_SIZE);
+ if (ret != n * BDRV_SECTOR_SIZE) {
return -1;
Please make that "return ret" (and possibly "if (ret < 0)", if you want to).
