[Qemu-devel] [PATCH 4/5] block: cow - used QEMU_PACKED for on-disk structures
cow_header_v2 is read and written directly from the image file
with bdrv_pread()/bdrv_pwrite(), and as such should be packed to
avoid unintentional padding.
Also change struct cow_header_v2 to a typedef, and some minor
code style changes to keep checkpatch.pl happy.
Signed-off-by: Jeff Cody
---
block/cow.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/block/cow.c b/block/cow.c
index 909c3e7..9c15afb 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -32,14 +32,14 @@
#define COW_MAGIC 0x4f4f4f4d /* MOOO */
#define COW_VERSION 2
-struct cow_header_v2 {
+typedef struct QEMU_PACKED cow_header_v2 {
uint32_t magic;
uint32_t version;
char backing_file[1024];
int32_t mtime;
uint64_t size;
uint32_t sectorsize;
-};
+} COWHeaderV2;
typedef struct BDRVCowState {
CoMutex lock;
@@ -48,21 +48,22 @@ typedef struct BDRVCowState {
static int cow_probe(const uint8_t *buf, int buf_size, const char *filename)
{
-const struct cow_header_v2 *cow_header = (const void *)buf;
+const COWHeaderV2 *cow_header = (const void *)buf;
-if (buf_size >= sizeof(struct cow_header_v2) &&
+if (buf_size >= sizeof(COWHeaderV2) &&
be32_to_cpu(cow_header->magic) == COW_MAGIC &&
-be32_to_cpu(cow_header->version) == COW_VERSION)
+be32_to_cpu(cow_header->version) == COW_VERSION) {
return 100;
-else
+} else {
return 0;
+}
}
static int cow_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVCowState *s = bs->opaque;
-struct cow_header_v2 cow_header;
+COWHeaderV2 cow_header;
int bitmap_size;
int64_t size;
int ret;
@@ -109,7 +110,7 @@ static int cow_open(BlockDriverState *bs, QDict *options,
int flags,
*/
static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum, bool
*first)
{
-uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
+uint64_t offset = sizeof(COWHeaderV2) + bitnum / 8;
uint8_t bitmap;
int ret;
@@ -172,7 +173,7 @@ static int cow_find_streak(const uint8_t *bitmap, int
value, int start, int nb_s
static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, int *num_same)
{
-int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8;
+int64_t bitnum = sector_num + sizeof(COWHeaderV2) * 8;
uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE;
uint8_t bitmap[BDRV_SECTOR_SIZE];
int ret;
@@ -298,7 +299,7 @@ static void cow_close(BlockDriverState *bs)
static int cow_create(const char *filename, QEMUOptionParameter *options,
Error **errp)
{
-struct cow_header_v2 cow_header;
+COWHeaderV2 cow_header;
struct stat st;
int64_t image_sectors = 0;
const char *image_filename = NULL;
--
1.8.3.1
Re: [Qemu-devel] [PATCH 4/5] block: cow - used QEMU_PACKED for on-disk structures
On 09/19/2013 11:43 AM, Jeff Cody wrote:
> cow_header_v2 is read and written directly from the image file
> with bdrv_pread()/bdrv_pwrite(), and as such should be packed to
> avoid unintentional padding.
>
> Also change struct cow_header_v2 to a typedef, and some minor
> code style changes to keep checkpatch.pl happy.
>
> Signed-off-by: Jeff Cody
> ---
> block/cow.c | 21 +++--
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/block/cow.c b/block/cow.c
> index 909c3e7..9c15afb 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -32,14 +32,14 @@
> #define COW_MAGIC 0x4f4f4f4d /* MOOO */
> #define COW_VERSION 2
>
> -struct cow_header_v2 {
> +typedef struct QEMU_PACKED cow_header_v2 {
> uint32_t magic;
> uint32_t version;
> char backing_file[1024];
> int32_t mtime;
> uint64_t size;
> uint32_t sectorsize;
> -};
> +} COWHeaderV2;
This changes the layout of this struct. In particular, there's padding
(depending on the host) between mtime and size.
I don't know what the right solution is: COWHeaderV3 with the bug fix, leaving
V2 alone; adding an int32_t dummy there where the padding was; nothing,
considering the padding to be gone a good thing.
r~
Re: [Qemu-devel] [PATCH 4/5] block: cow - used QEMU_PACKED for on-disk structures
On Thu, Sep 19, 2013 at 12:01:24PM -0700, Richard Henderson wrote:
> On 09/19/2013 11:43 AM, Jeff Cody wrote:
> > cow_header_v2 is read and written directly from the image file
> > with bdrv_pread()/bdrv_pwrite(), and as such should be packed to
> > avoid unintentional padding.
> >
> > Also change struct cow_header_v2 to a typedef, and some minor
> > code style changes to keep checkpatch.pl happy.
> >
> > Signed-off-by: Jeff Cody
> > ---
> > block/cow.c | 21 +++--
> > 1 file changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/block/cow.c b/block/cow.c
> > index 909c3e7..9c15afb 100644
> > --- a/block/cow.c
> > +++ b/block/cow.c
> > @@ -32,14 +32,14 @@
> > #define COW_MAGIC 0x4f4f4f4d /* MOOO */
> > #define COW_VERSION 2
> >
> > -struct cow_header_v2 {
> > +typedef struct QEMU_PACKED cow_header_v2 {
> > uint32_t magic;
> > uint32_t version;
> > char backing_file[1024];
> > int32_t mtime;
> > uint64_t size;
> > uint32_t sectorsize;
> > -};
> > +} COWHeaderV2;
>
> This changes the layout of this struct. In particular, there's padding
> (depending on the host) between mtime and size.
>
You are right, and that poses a problem for this patch.
> I don't know what the right solution is: COWHeaderV3 with the bug fix, leaving
> V2 alone; adding an int32_t dummy there where the padding was; nothing,
> considering the padding to be gone a good thing.
>
I'm not sure either. I don't think the right thing is to take the
patch as-is, because that will likely break a lot of existing COW
images (I just checked, and on x86_64, it is 1056 bytes unpacked, or
1048 bytes packed).
Unfortunately, this means that theoretically, image files with this
format may not be portable, depending on the hosts' compiler and
alignment. In reality, it likely is not a problem.
I'll drop this one for v2.
Re: [Qemu-devel] [PATCH 4/5] block: cow - used QEMU_PACKED for on-disk structures
Jeff Cody writes:
> On Thu, Sep 19, 2013 at 12:01:24PM -0700, Richard Henderson wrote:
>> On 09/19/2013 11:43 AM, Jeff Cody wrote:
>> > cow_header_v2 is read and written directly from the image file
>> > with bdrv_pread()/bdrv_pwrite(), and as such should be packed to
>> > avoid unintentional padding.
>> >
>> > Also change struct cow_header_v2 to a typedef, and some minor
>> > code style changes to keep checkpatch.pl happy.
>> >
>> > Signed-off-by: Jeff Cody
>> > ---
>> > block/cow.c | 21 +++--
>> > 1 file changed, 11 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/block/cow.c b/block/cow.c
>> > index 909c3e7..9c15afb 100644
>> > --- a/block/cow.c
>> > +++ b/block/cow.c
>> > @@ -32,14 +32,14 @@
>> > #define COW_MAGIC 0x4f4f4f4d /* MOOO */
>> > #define COW_VERSION 2
>> >
>> > -struct cow_header_v2 {
>> > +typedef struct QEMU_PACKED cow_header_v2 {
>> > uint32_t magic;
>> > uint32_t version;
>> > char backing_file[1024];
>> > int32_t mtime;
>> > uint64_t size;
>> > uint32_t sectorsize;
>> > -};
>> > +} COWHeaderV2;
>>
>> This changes the layout of this struct. In particular, there's padding
>> (depending on the host) between mtime and size.
>>
>
> You are right, and that poses a problem for this patch.
>
>> I don't know what the right solution is: COWHeaderV3 with the bug fix,
>> leaving
>> V2 alone; adding an int32_t dummy there where the padding was; nothing,
>> considering the padding to be gone a good thing.
>>
>
> I'm not sure either. I don't think the right thing is to take the
> patch as-is, because that will likely break a lot of existing COW
> images (I just checked, and on x86_64, it is 1056 bytes unpacked, or
> 1048 bytes packed).
>
> Unfortunately, this means that theoretically, image files with this
> format may not be portable, depending on the hosts' compiler and
> alignment. In reality, it likely is not a problem.
>
> I'll drop this one for v2.
Possible solutions:
* Declare format "cow" non-portable. To move a cow to another system,
you have to convert to a portable format.
* Keep using the non-portable header. When opening an existing image,
guess which of the two header variants it got: the padding should be
zero, size and sectorsize sane. Perhaps provide an option to overrule
the guess.
Who's still using format "cow"?
Re: [Qemu-devel] [PATCH 4/5] block: cow - used QEMU_PACKED for on-disk structures
On Fri, Sep 20, 2013 at 08:23:54AM +0200, Markus Armbruster wrote:
> Jeff Cody writes:
>
> > On Thu, Sep 19, 2013 at 12:01:24PM -0700, Richard Henderson wrote:
> >> On 09/19/2013 11:43 AM, Jeff Cody wrote:
> >> > cow_header_v2 is read and written directly from the image file
> >> > with bdrv_pread()/bdrv_pwrite(), and as such should be packed to
> >> > avoid unintentional padding.
> >> >
> >> > Also change struct cow_header_v2 to a typedef, and some minor
> >> > code style changes to keep checkpatch.pl happy.
> >> >
> >> > Signed-off-by: Jeff Cody
> >> > ---
> >> > block/cow.c | 21 +++--
> >> > 1 file changed, 11 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/block/cow.c b/block/cow.c
> >> > index 909c3e7..9c15afb 100644
> >> > --- a/block/cow.c
> >> > +++ b/block/cow.c
> >> > @@ -32,14 +32,14 @@
> >> > #define COW_MAGIC 0x4f4f4f4d /* MOOO */
> >> > #define COW_VERSION 2
> >> >
> >> > -struct cow_header_v2 {
> >> > +typedef struct QEMU_PACKED cow_header_v2 {
> >> > uint32_t magic;
> >> > uint32_t version;
> >> > char backing_file[1024];
> >> > int32_t mtime;
> >> > uint64_t size;
> >> > uint32_t sectorsize;
> >> > -};
> >> > +} COWHeaderV2;
> >>
> >> This changes the layout of this struct. In particular, there's padding
> >> (depending on the host) between mtime and size.
> >>
> >
> > You are right, and that poses a problem for this patch.
> >
> >> I don't know what the right solution is: COWHeaderV3 with the bug fix,
> >> leaving
> >> V2 alone; adding an int32_t dummy there where the padding was; nothing,
> >> considering the padding to be gone a good thing.
> >>
> >
> > I'm not sure either. I don't think the right thing is to take the
> > patch as-is, because that will likely break a lot of existing COW
> > images (I just checked, and on x86_64, it is 1056 bytes unpacked, or
> > 1048 bytes packed).
> >
> > Unfortunately, this means that theoretically, image files with this
> > format may not be portable, depending on the hosts' compiler and
> > alignment. In reality, it likely is not a problem.
> >
> > I'll drop this one for v2.
>
> Possible solutions:
>
> * Declare format "cow" non-portable. To move a cow to another system,
> you have to convert to a portable format.
>
I favor this approach, especially since "cow" is not likely to be used
in new environments.
> * Keep using the non-portable header. When opening an existing image,
> guess which of the two header variants it got: the padding should be
> zero, size and sectorsize sane. Perhaps provide an option to overrule
> the guess.
>
> Who's still using format "cow"?
>
Re: [Qemu-devel] [PATCH 4/5] block: cow - used QEMU_PACKED for on-disk structures
Am 25.09.2013 um 17:12 hat Jeff Cody geschrieben:
> On Fri, Sep 20, 2013 at 08:23:54AM +0200, Markus Armbruster wrote:
> > Jeff Cody writes:
> >
> > > On Thu, Sep 19, 2013 at 12:01:24PM -0700, Richard Henderson wrote:
> > >> On 09/19/2013 11:43 AM, Jeff Cody wrote:
> > >> > cow_header_v2 is read and written directly from the image file
> > >> > with bdrv_pread()/bdrv_pwrite(), and as such should be packed to
> > >> > avoid unintentional padding.
> > >> >
> > >> > Also change struct cow_header_v2 to a typedef, and some minor
> > >> > code style changes to keep checkpatch.pl happy.
> > >> >
> > >> > Signed-off-by: Jeff Cody
> > >> > ---
> > >> > block/cow.c | 21 +++--
> > >> > 1 file changed, 11 insertions(+), 10 deletions(-)
> > >> >
> > >> > diff --git a/block/cow.c b/block/cow.c
> > >> > index 909c3e7..9c15afb 100644
> > >> > --- a/block/cow.c
> > >> > +++ b/block/cow.c
> > >> > @@ -32,14 +32,14 @@
> > >> > #define COW_MAGIC 0x4f4f4f4d /* MOOO */
> > >> > #define COW_VERSION 2
> > >> >
> > >> > -struct cow_header_v2 {
> > >> > +typedef struct QEMU_PACKED cow_header_v2 {
> > >> > uint32_t magic;
> > >> > uint32_t version;
> > >> > char backing_file[1024];
> > >> > int32_t mtime;
> > >> > uint64_t size;
> > >> > uint32_t sectorsize;
> > >> > -};
> > >> > +} COWHeaderV2;
> > >>
> > >> This changes the layout of this struct. In particular, there's padding
> > >> (depending on the host) between mtime and size.
> > >>
> > >
> > > You are right, and that poses a problem for this patch.
> > >
> > >> I don't know what the right solution is: COWHeaderV3 with the bug fix,
> > >> leaving
> > >> V2 alone; adding an int32_t dummy there where the padding was; nothing,
> > >> considering the padding to be gone a good thing.
> > >>
> > >
> > > I'm not sure either. I don't think the right thing is to take the
> > > patch as-is, because that will likely break a lot of existing COW
> > > images (I just checked, and on x86_64, it is 1056 bytes unpacked, or
> > > 1048 bytes packed).
> > >
> > > Unfortunately, this means that theoretically, image files with this
> > > format may not be portable, depending on the hosts' compiler and
> > > alignment. In reality, it likely is not a problem.
> > >
> > > I'll drop this one for v2.
> >
> > Possible solutions:
> >
> > * Declare format "cow" non-portable. To move a cow to another system,
> > you have to convert to a portable format.
> >
>
> I favor this approach, especially since "cow" is not likely to be used
> in new environments.
But COW isn't a native qemu format. We should do whatever the format
really requires, so that new qemu versions handle it correctly - and if
old qemu versions produced corrupted files, bad luck.
Kevin
Re: [Qemu-devel] [PATCH 4/5] block: cow - used QEMU_PACKED for on-disk structures
On Wed, Sep 25, 2013 at 07:25:20PM +0200, Kevin Wolf wrote:
> Am 25.09.2013 um 17:12 hat Jeff Cody geschrieben:
> > On Fri, Sep 20, 2013 at 08:23:54AM +0200, Markus Armbruster wrote:
> > > Jeff Cody writes:
> > >
> > > > On Thu, Sep 19, 2013 at 12:01:24PM -0700, Richard Henderson wrote:
> > > >> On 09/19/2013 11:43 AM, Jeff Cody wrote:
> > > >> > cow_header_v2 is read and written directly from the image file
> > > >> > with bdrv_pread()/bdrv_pwrite(), and as such should be packed to
> > > >> > avoid unintentional padding.
> > > >> >
> > > >> > Also change struct cow_header_v2 to a typedef, and some minor
> > > >> > code style changes to keep checkpatch.pl happy.
> > > >> >
> > > >> > Signed-off-by: Jeff Cody
> > > >> > ---
> > > >> > block/cow.c | 21 +++--
> > > >> > 1 file changed, 11 insertions(+), 10 deletions(-)
> > > >> >
> > > >> > diff --git a/block/cow.c b/block/cow.c
> > > >> > index 909c3e7..9c15afb 100644
> > > >> > --- a/block/cow.c
> > > >> > +++ b/block/cow.c
> > > >> > @@ -32,14 +32,14 @@
> > > >> > #define COW_MAGIC 0x4f4f4f4d /* MOOO */
> > > >> > #define COW_VERSION 2
> > > >> >
> > > >> > -struct cow_header_v2 {
> > > >> > +typedef struct QEMU_PACKED cow_header_v2 {
> > > >> > uint32_t magic;
> > > >> > uint32_t version;
> > > >> > char backing_file[1024];
> > > >> > int32_t mtime;
> > > >> > uint64_t size;
> > > >> > uint32_t sectorsize;
> > > >> > -};
> > > >> > +} COWHeaderV2;
> > > >>
> > > >> This changes the layout of this struct. In particular, there's padding
> > > >> (depending on the host) between mtime and size.
> > > >>
> > > >
> > > > You are right, and that poses a problem for this patch.
> > > >
> > > >> I don't know what the right solution is: COWHeaderV3 with the bug fix,
> > > >> leaving
> > > >> V2 alone; adding an int32_t dummy there where the padding was; nothing,
> > > >> considering the padding to be gone a good thing.
> > > >>
> > > >
> > > > I'm not sure either. I don't think the right thing is to take the
> > > > patch as-is, because that will likely break a lot of existing COW
> > > > images (I just checked, and on x86_64, it is 1056 bytes unpacked, or
> > > > 1048 bytes packed).
> > > >
> > > > Unfortunately, this means that theoretically, image files with this
> > > > format may not be portable, depending on the hosts' compiler and
> > > > alignment. In reality, it likely is not a problem.
> > > >
> > > > I'll drop this one for v2.
> > >
> > > Possible solutions:
> > >
> > > * Declare format "cow" non-portable. To move a cow to another system,
> > > you have to convert to a portable format.
> > >
> >
> > I favor this approach, especially since "cow" is not likely to be used
> > in new environments.
>
> But COW isn't a native qemu format. We should do whatever the format
> really requires, so that new qemu versions handle it correctly - and if
> old qemu versions produced corrupted files, bad luck.
>
It is from UML, right? Is there an official spec that is still around
(most of the links I have found suffer from link rot)? The closest I
could find to a spec were old UML patches for x86_64 that cleaned up
some data types, so that the following was defined:
struct cow_header_v2 {
__u32 magic;
__u32 version;
char backing_file[PATH_LEN_V2];
time_t mtime;
__u64 size;
int sectorsize;
};
That remains ambiguous, although given the era I suppose it could be
argued that 32-bit architecture and alignment is assumed. But if this
is the original spec, then it seems like a non-portable one.
Re: [Qemu-devel] [PATCH 4/5] block: cow - used QEMU_PACKED for on-disk structures
On 09/25/2013 12:01 PM, Jeff Cody wrote:
> It is from UML, right? Is there an official spec that is still around
> (most of the links I have found suffer from link rot)? The closest I
> could find to a spec were old UML patches for x86_64 that cleaned up
> some data types, so that the following was defined:
>
> struct cow_header_v2 {
>
>__u32 magic;
>__u32 version;
>char backing_file[PATH_LEN_V2];
>time_t mtime;
>__u64 size;
>int sectorsize;
> };
>
> That remains ambiguous, although given the era I suppose it could be
> argued that 32-bit architecture and alignment is assumed. But if this
> is the original spec, then it seems like a non-portable one.
Indeed. Not just the potential padding there, but of course
the size of time_t varies between hosts. And since we use
an int32_t not time_t, we're not even necessarily compatible
with UML.
r~
