[Qemu-devel] [PATCH 0/5] block: use QEMU_PACKED for on-disk structures

[Qemu-devel] [PATCH 0/5] block: use QEMU_PACKED for on-disk structures

[Jeff Cody]

Several block image formats did not consistently use packed attributes
when directly reading / writing structures from disk (mainly image format
headers).

These series updates the image formats (see list below), to use
QEMU_PACKED for on-disk structs.  (Some minor code cleanup may also
have ensued, to keep checkpatch.pl happy)

Jeff Cody (5):
  block: vdi - use QEMU_PACKED for on-disk structures
  block: vpc - use QEMU_PACKED for on-disk structures
  block: qcow2 - used QEMU_PACKED for on-disk structures
  block: cow - used QEMU_PACKED for on-disk structures
  block: qed - use QEMU_PACKED for on-disk structures

 block/cow.c   | 21 +++++++++++----------
 block/qcow2.c |  2 +-
 block/qcow2.h |  2 +-
 block/qed.h   |  2 +-
 block/vdi.c   |  2 +-
 block/vpc.c   | 28 ++++++++++++++--------------
 6 files changed, 29 insertions(+), 28 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/5] block: vdi - use QEMU_PACKED for on-disk structures

[Jeff Cody]

The header struct VdiHeader is an on-disk structure for the image
format, and as such should be packed.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vdi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vdi.c b/block/vdi.c
index dcbc27c..f8bdc92 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -140,7 +140,7 @@ static inline void uuid_unparse(const uuid_t uu, char *out)
 }
 #endif
 
-typedef struct {
+typedef struct QEMU_PACKED {
     char text[0x40];
     uint32_t signature;
     uint32_t version;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/5] block: vpc - use QEMU_PACKED for on-disk structures

[Jeff Cody]

The VHD footer and header structs (vhd_footer and vhd_dyndisk_header)
are on-disk structures for the image format, and as such should be
packed.

Go ahead and make these typedefs as well, with the preferred QEMU
naming convention, so that the packed attribute is used consistently
with the struct.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vpc.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index db61274..5daca43 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -46,7 +46,7 @@ enum vhd_type {
 #define VHD_TIMESTAMP_BASE 946684800
 
 // always big-endian
-struct vhd_footer {
+typedef struct QEMU_PACKED vhd_footer {
     char        creator[8]; // "conectix"
     uint32_t    features;
     uint32_t    version;
@@ -79,9 +79,9 @@ struct vhd_footer {
     uint8_t     uuid[16];
 
     uint8_t     in_saved_state;
-};
+} VHDFooter;
 
-struct vhd_dyndisk_header {
+typedef struct QEMU_PACKED vhd_dyndisk_header {
     char        magic[8]; // "cxsparse"
 
     // Offset of next header structure, 0xFFFFFFFF if none
@@ -111,7 +111,7 @@ struct vhd_dyndisk_header {
         uint32_t    reserved;
         uint64_t    data_offset;
     } parent_locator[8];
-};
+} VHDDynDiskHeader;
 
 typedef struct BDRVVPCState {
     CoMutex lock;
@@ -160,8 +160,8 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVVPCState *s = bs->opaque;
     int i;
-    struct vhd_footer* footer;
-    struct vhd_dyndisk_header* dyndisk_header;
+    VHDFooter *footer;
+    VHDDynDiskHeader *dyndisk_header;
     uint8_t buf[HEADER_SIZE];
     uint32_t checksum;
     int disk_type = VHD_DYNAMIC;
@@ -172,7 +172,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    footer = (struct vhd_footer*) s->footer_buf;
+    footer = (VHDFooter *) s->footer_buf;
     if (strncmp(footer->creator, "conectix", 8)) {
         int64_t offset = bdrv_getlength(bs->file);
         if (offset < 0) {
@@ -224,7 +224,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }
 
-        dyndisk_header = (struct vhd_dyndisk_header *) buf;
+        dyndisk_header = (VHDDynDiskHeader *) buf;
 
         if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
             ret = -EINVAL;
@@ -446,7 +446,7 @@ static int vpc_read(BlockDriverState *bs, int64_t sector_num,
     int ret;
     int64_t offset;
     int64_t sectors, sectors_per_block;
-    struct vhd_footer *footer = (struct vhd_footer *) s->footer_buf;
+    VHDFooter *footer = (VHDFooter *) s->footer_buf;
 
     if (cpu_to_be32(footer->type) == VHD_FIXED) {
         return bdrv_read(bs->file, sector_num, buf, nb_sectors);
@@ -495,7 +495,7 @@ static int vpc_write(BlockDriverState *bs, int64_t sector_num,
     int64_t offset;
     int64_t sectors, sectors_per_block;
     int ret;
-    struct vhd_footer *footer =  (struct vhd_footer *) s->footer_buf;
+    VHDFooter *footer =  (VHDFooter *) s->footer_buf;
 
     if (cpu_to_be32(footer->type) == VHD_FIXED) {
         return bdrv_write(bs->file, sector_num, buf, nb_sectors);
@@ -597,8 +597,8 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
 
 static int create_dynamic_disk(int fd, uint8_t *buf, int64_t total_sectors)
 {
-    struct vhd_dyndisk_header* dyndisk_header =
-        (struct vhd_dyndisk_header*) buf;
+    VHDDynDiskHeader *dyndisk_header =
+        (VHDDynDiskHeader *) buf;
     size_t block_size, num_bat_entries;
     int i;
     int ret = -EIO;
@@ -688,7 +688,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options,
                       Error **errp)
 {
     uint8_t buf[1024];
-    struct vhd_footer *footer = (struct vhd_footer *) buf;
+    VHDFooter *footer = (VHDFooter *) buf;
     QEMUOptionParameter *disk_type_param;
     int fd, i;
     uint16_t cyls = 0;
@@ -791,7 +791,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options,
 static int vpc_has_zero_init(BlockDriverState *bs)
 {
     BDRVVPCState *s = bs->opaque;
-    struct vhd_footer *footer =  (struct vhd_footer *) s->footer_buf;
+    VHDFooter *footer =  (VHDFooter *) s->footer_buf;
 
     if (cpu_to_be32(footer->type) == VHD_FIXED) {
         return bdrv_has_zero_init(bs->file);
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/5] block: qcow2 - used QEMU_PACKED for on-disk structures

[Jeff Cody]

QCowHeader and QCowExtension are structs that reside in the on-disk
image format, and are read and written directly via bdrv_pread()/write(),
and as such should be packed to avoid any unintentional struct padding.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/qcow2.c | 2 +-
 block/qcow2.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 318d95d..02b92aa 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -49,7 +49,7 @@
 */
 
 
-typedef struct {
+typedef struct QEMU_PACKED {
     uint32_t magic;
     uint32_t len;
 } QCowExtension;
diff --git a/block/qcow2.h b/block/qcow2.h
index c90e5d6..1fe111a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -64,7 +64,7 @@
 #define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot"
 #define QCOW2_OPT_DISCARD_OTHER "pass-discard-other"
 
-typedef struct QCowHeader {
+typedef struct QEMU_PACKED QCowHeader {
     uint32_t magic;
     uint32_t version;
     uint64_t backing_file_offset;
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/5] block: cow - used QEMU_PACKED for on-disk structures

[Jeff Cody]

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 <jcody@redhat.com>
---
 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

[Richard Henderson]

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 <jcody@redhat.com>
> ---
>  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

[Jeff Cody]

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 <jcody@redhat.com>
> > ---
> >  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

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> 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 <jcody@redhat.com>
>> > ---
>> >  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

[Jeff Cody]

On Fri, Sep 20, 2013 at 08:23:54AM +0200, Markus Armbruster wrote:
> Jeff Cody <jcody@redhat.com> 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 <jcody@redhat.com>
> >> > ---
> >> >  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

[Kevin Wolf]

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 <jcody@redhat.com> 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 <jcody@redhat.com>
> > >> > ---
> > >> >  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

[Jeff Cody]

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 <jcody@redhat.com> 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 <jcody@redhat.com>
> > > >> > ---
> > > >> >  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

[Richard Henderson]

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~

[Qemu-devel] [PATCH 5/5] block: qed - use QEMU_PACKED for on-disk structures

[Jeff Cody]

QEDHeader is read, and written, directly from on-disk images
via bdrv_pread()/write().  To avoid any unintentional padding,
these structs should be packed.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/qed.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qed.h b/block/qed.h
index 2b4dded..4d6c812 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -83,7 +83,7 @@ enum {
     QED_NEED_CHECK_TIMEOUT = 5,    /* in seconds */
 };
 
-typedef struct {
+typedef struct QEMU_PACKED {
     uint32_t magic;                 /* QED\0 */
 
     uint32_t cluster_size;          /* in bytes */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/5] block: use QEMU_PACKED for on-disk structures

[Kevin Wolf]

Am 19.09.2013 um 20:43 hat Jeff Cody geschrieben:
> Several block image formats did not consistently use packed attributes
> when directly reading / writing structures from disk (mainly image format
> headers).
> 
> These series updates the image formats (see list below), to use
> QEMU_PACKED for on-disk structs.  (Some minor code cleanup may also
> have ensued, to keep checkpatch.pl happy)

One comment on coding style: It seems to be more common to have the
QEMU_PACKED attribute after the fields, i.e.:

    struct foo {
        ...
    } QEMU_PACKED;

Kevin