On Fri, Jul 25, 2014 at 07:51:47AM +0400, Denis V. Lunev wrote:
On 24/07/14 23:25, Jeff Cody wrote:
On Tue, Jul 22, 2014 at 05:19:37PM +0400, Denis V. Lunev wrote:
Parallels has released in the recent updates of Parallels Server 5/6
new addition to his image format. Images with signature WithouFreSpacExt
have offsets in the catalog coded not as offsets in sectors (multiple
of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
In this case all 64 bits of header->nb_sectors are used for image size.
This patch implements support of this for qemu-img.
Signed-off-by: Denis V. Lunev <address@hidden>
CC: Kevin Wolf <address@hidden>
CC: Stefan Hajnoczi <address@hidden>
---
block/parallels.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 02739cf..d9cb04f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -30,6 +30,7 @@
/**************************************************************/
#define HEADER_MAGIC "WithoutFreeSpace"
+#define HEADER_MAGIC2 "WithouFreSpacExt"
#define HEADER_VERSION 2
#define HEADER_SIZE 64
@@ -54,6 +55,8 @@ typedef struct BDRVParallelsState {
unsigned int catalog_size;
unsigned int tracks;
+
+ unsigned int off_multiplier;
} BDRVParallelsState;
static int parallels_probe(const uint8_t *buf, int buf_size, const char
*filename)
@@ -63,7 +66,8 @@ static int parallels_probe(const uint8_t *buf, int buf_size,
const char *filenam
if (buf_size < HEADER_SIZE)
return 0;
- if (!memcmp(ph->magic, HEADER_MAGIC, 16) &&
+ if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
+ !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
(le32_to_cpu(ph->version) == HEADER_VERSION))
return 100;
@@ -85,13 +89,18 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
goto fail;
}
+ bs->total_sectors = le64_to_cpu(ph.nb_sectors);
+
bs->total_sectors is a int64_t, and ph.nb_sectors is a uint64_t.
Should we do a sanity check here on the max number of sectors?
in reality such image will not be usable as we will later have to multiply
this count with 512 to calculate offset in the file
if (le32_to_cpu(ph.version) != HEADER_VERSION)
goto fail_format;
- if (memcmp(ph.magic, HEADER_MAGIC, 16))
+ if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
+ s->off_multiplier = 1;
+ bs->total_sectors = (uint32_t)bs->total_sectors;
(same comment as in patch 1, w.r.t. cast vs. bitmask)
ok
+ } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16))
+ s->off_multiplier = le32_to_cpu(ph.tracks);
Is there a maximum size in the specification for ph.tracks?
no, there is no limitation. Though in reality I have seen the
following images only:
252 sectors, 63 sectors, 256 sectors, 2048 sectors
and only 2048 was used with this new header.
This is just an observation.
+ else
goto fail_format;
(same comment as the last patch, w.r.t. braces)
- bs->total_sectors = (uint32_t)le64_to_cpu(ph.nb_sectors);
-
s->tracks = le32_to_cpu(ph.tracks);
if (s->tracks == 0) {
error_setg(errp, "Invalid image: Zero sectors per track");
@@ -137,7 +146,8 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t
sector_num)
/* not allocated */
if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
return -1;
- return (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
+ return
+ ((uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset) *
512;
Do we need to worry about overflow here, depending on the value of
off_multiplier?
Also, (and this existed prior to this patch), - we are casting to
uint64_t, although the function returns int64_t.
good catch. I'll change the declaration to uint64_t and will return 0
from previous if aka
Thanks.
I'm not sure that is the best option though - the only place the
return value from this function is used is in parallels_read(), which
passes the output into bdrv_pread() as the offset. The offset
parameter for bdrv_pread/pwrite is an int64_t, not a uint64_t.
if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
It is not possible to obtain 0 as a real offset of any sector as
this is an offset of the header.
Regarding overflow check. Do you think that we should return
error to the upper layer or we could silently fill result data
with zeroes as was done originally for the following case
'index > s->catalog_size' ?
I think it would be best to return error (anything < 0 will also cause
bdrv_pread to return -EIO, and it is also checked for in
parallels_read).
I also don't mean to over-complicate things here for you, which is why
I asked about the max value of ph.tracks. If that has a reasonable
bounds check, then we don't need to worry about overflow at all, and
can just change the cast to an int64_t instead of uint64_t.
I think we are safe so long as ph.tracks <= (INT32_MAX/513), and it
that seems like it would be a reasonably flexible bounds limit check
when first opening the image (since QEMU can't really support higher
than