| Message ID | 1406035177-221890-5-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
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 <den@openvz.org> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Stefan Hajnoczi <stefanha@redhat.com> > --- > 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? > 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) > + } 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? > + 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. > } > > static int parallels_read(BlockDriverState *bs, int64_t sector_num, > -- > 1.9.1 > >
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 <den@openvz.org> >> CC: Kevin Wolf <kwolf@redhat.com> >> CC: Stefan Hajnoczi <stefanha@redhat.com> >> --- >> 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 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' ? >> } >> >> static int parallels_read(BlockDriverState *bs, int64_t sector_num, >> -- >> 1.9.1 >> >>
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 <den@openvz.org> > >>CC: Kevin Wolf <kwolf@redhat.com> > >>CC: Stefan Hajnoczi <stefanha@redhat.com> > >>--- > >> 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 that, anyway). Thanks, Jeff > >> } > >> static int parallels_read(BlockDriverState *bs, int64_t sector_num, > >>-- > >>1.9.1 > >> > >> >
On 25/07/14 17:08, Jeff Cody wrote: > 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 <den@openvz.org> >>>> CC: Kevin Wolf <kwolf@redhat.com> >>>> CC: Stefan Hajnoczi <stefanha@redhat.com> >>>> --- >>>> 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 sounds good to me Thank you for an idea :) > that, anyway). > > Thanks, > Jeff > >>>> } >>>> static int parallels_read(BlockDriverState *bs, int64_t sector_num, >>>> -- >>>> 1.9.1 >>>> >>>>
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); + 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; + } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) + s->off_multiplier = le32_to_cpu(ph.tracks); + else goto fail_format; - 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; } static int parallels_read(BlockDriverState *bs, int64_t sector_num,
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 <den@openvz.org> CC: Kevin Wolf <kwolf@redhat.com> CC: Stefan Hajnoczi <stefanha@redhat.com> --- block/parallels.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)